Skip to content

Conversation

@20162026
Copy link
Collaborator

@20162026 20162026 commented Jun 9, 2025

fix #91

tested with g++-15 (Ubuntu 15-20250404-0ubuntu1) 15.0.1 20250404 (experimental) [master r15-9193-g08e803aa9be] on ubuntu:rolling docker img

Im not sure if this approach is correct or why the previous impl used const container for const T storage

@20162026 20162026 requested a review from a team June 9, 2025 16:26
@20162026
Copy link
Collaborator Author

20162026 commented Jun 9, 2025

Also, should beman::inplace_vector<const int, 3> cvec{1, 2, 3}; compile? Because right now it fails both in main and with this pr

Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Thanks for the PR. I will test this once I have gcc-15 fully working on my environment.

It is currently spilling out this warning when compiling, though I question if this is actually due to this PR.

/home/.../beman/inplace_vector/tests/beman/inplace_vector/modifiers.test.cpp: In member function 'void {anonymous}::Modifiers_EraseRange_Test<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = TestParam<NonTriviallyDefaultConstructible, 1>]':
....
/home/linuxbrew/.linuxbrew/Cellar/gcc/15.1.0/include/c++/15/bits/stl_algobase.h:426:32: warning: 'void* __builtin_memmove(void*, const void*, long unsigned int)' reading between 5 and 1016 bytes from a region of size 4 [-Wstringop-overread]
426 | __builtin_memmove(_GLIBCXX_TO_ADDR(__result),
| ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
427 | _GLIBCXX_TO_ADDR(__first),
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
428 | __n * sizeof(*first));
| ~~~~~~~~~~~~~~~~~~~~~~~
/home/.../beman/inplace_vector/tests/beman/inplace_vector/modifiers.test.cpp: In member function 'void {anonymous}::Modifiers_EraseRange_Test<gtest_TypeParam
>::TestBody() [with gtest_TypeParam
= TestParam<Trivial, 1>]':
/home/.../beman/inplace_vector/tests/beman/inplace_vector/modifiers.test.cpp:845:6: note: at offset 4 into source object 'device' of size 8
845 | IV device(reference);
| ^~~~~~

@wusatosi
Copy link
Member

wusatosi commented Jun 9, 2025

Also, should beman::inplace_vector<const int, 3> cvec{1, 2, 3}; compile? Because right now it fails both in main and with this pr

It should

@20162026
Copy link
Collaborator Author

Also, should beman::inplace_vector<const int, 3> cvec{1, 2, 3}; compile? Because right now it fails both in main and with this pr

It should

Then I guess we'll need to review the current implementation because it pretty much prevents using most constructors or functions for const T. But this is outside the scope of this PR...

also what compiler settings did you use when you got the warning with 15.1?

@wusatosi
Copy link
Member

also what compiler settings did you use when you got the warning with 15.1?

My installation of gcc might be buggy. We will have gcc-15 support in our CI soon(?), let's resolve this when we have gcc-15 support ready.

@JeffGarland
Copy link
Member

JeffGarland commented Jun 29, 2025

Also, should beman::inplace_vector<const int, 3> cvec{1, 2, 3}; compile? Because right now it fails both in main and with this pr

It should

Then I guess we'll need to review the current implementation because it pretty much prevents using most constructors or functions for const T. But this is outside the scope of this PR...

git status
On branch fix/gcc15constt
Your branch is up to date with 'origin/fix/gcc15constt'.

g++-15 (Ubuntu 15-20250404-0ubuntu1) 15.0.1 20250404 (experimental) [master r15-9193-g08e803aa9be]

cmake .. -DCMAKE_CXX_COMPILER=g++-15 -DCMAKE_CXX_STANDARD=23 -G=Ninja

same compiler as you and I'm able to compile your branch in both 20 and 23 mode with no warnings. In 26 mode we start getting warnings about is_trivial_v being deprecated. For fun also compiled and tested with g++-14, clang-20 with no issues. So not sure why you're having an issue?

@JeffGarland
Copy link
Member

no issues. So not sure why you're having an issue?

oops I see -- you must have made a change on the branch that stops the error:

/usr/include/c++/15/bits/stl_construct.h:99:21: error: invalid conversion from ‘const void*’ to ‘void*’ [-fpermissive]
99 | void* __loc = __location;
| ^~~~~~~~~~
| |
| const void*

@JeffGarland
Copy link
Member

JeffGarland commented Jun 29, 2025

Thinking about this, I'm not seeing how const T can be valid for any standard container -- how would assign to a value in the first place since it's const. With vector it actually has a static_assert to prevent this case:

/opt/compiler-explorer/gcc-15.1.0/include/c++/15.1.0/bits/stl_vector.h: In instantiation of 'class std::vector<const int>':
<source>:13:28:   required from here
   13 |     std::vector<const int> ci { 1, 2, 3};
      |                            ^~
/opt/compiler-explorer/gcc-15.1.0/include/c++/15.1.0/bits/stl_vector.h:470:66: error: static assertion failed: std::vector must have a non-const, non-volatile value_type
  470 |       static_assert(is_same<typename remove_cv<_Tp>::type, _Tp>::value,

https://godbolt.org/z/johqKq3YW

I don't really see wording that forces this to be the case, but I might just be missing it...

tldr: I'd remove const as a case and consider adding a static assert.

@20162026
Copy link
Collaborator Author

20162026 commented Jun 30, 2025

the problem with inplace_vector is that I dont see any requirements in the paper that would prevent or allow using const T, while regular vector (if I'm not mistaken) requires T to be CopyAssignable.

I dont see any real world usecase of inplace_vector<const T, N> but dont think we should add a static_assert that is not required by the paper

@JeffGarland
Copy link
Member

That's specified in the general container requirements which apply to inplace_vector as well

https://eel.is/c++draft/container.requirements.general

@20162026
Copy link
Collaborator Author

@JeffGarland in the link you have provided, i only see Cpp17Erasable as mandatory requirement for inplace_vector type T, unless I'm missing smth?

@JeffGarland
Copy link
Member

@JeffGarland in the link you have provided, i only see Cpp17Erasable as mandatory requirement for inplace_vector type T, unless I'm missing smth?

You have to read the entire section and then some. To do anything useful with the container you need constructable or move assignable (can't move assign to a const object). Also, these requirements are littered about:

https://eel.is/c++draft/containers#inplace.vector.cons-4

I believe this is the same path to why vector gets to the static_assert on T (it's not explicitly one thing -- and remember inplace_vector emulates vector). Anyway, there's simply nothing useful that can be done with const elements (note the entire collection can be const -- but that's different).

@20162026
Copy link
Collaborator Author

As I understand std::array is also covered by general container req and all compilers I have tested allows const T as long as values ar initialized at compile time https://godbolt.org/z/a8K57cM5P

I dont mind preventing const T with static_assert as I can't think of any use case for it, but I'm still not convinced that this would be the correct approach based on the paper.

@JeffGarland
Copy link
Member

hmm, right -- we had to make this just impossible to discover the rules. You're correct that a constexpr initialization works -- but array is fixed sized with non of the dynamics of inplace_vector. Note that const isn't an option if you try to do much of anything with array either.

https://eel.is/c++draft/containers#array.cons-1

Also it clearly didn't start this way in c++11 https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0031r0.html

@20162026
Copy link
Collaborator Author

20162026 commented Jun 30, 2025

@JeffGarland maybe you could address this issue in one of the weekly Beman meetings? It might be helpful to get a majority consensus on how they would expect inplace_vector to behave in this situation or maybe someone is in touch with the author of the proposal...

@JeffGarland
Copy link
Member

Sorry we didn't have time to address at the sync -- and I saw this after the meeting.

@JeffGarland
Copy link
Member

This has been sitting forever. gcc 15 is in the CI fleet -- apologize for holding it up.

@JeffGarland JeffGarland merged commit e6c99e9 into bemanproject:main Dec 29, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ref_impl.text.cpp compilation fails g++-15

3 participants