-
Notifications
You must be signed in to change notification settings - Fork 11
Add freestanding build support #77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for your contribution! I will review once the PR is ready :) |
|
"-fno-exceptions" and freestanding are different things. Freestanding implementations are required to support exception handling. It looks like this patch is attempting to give this library "-fno-exceptions" support, which is not Standard C++ although used in practice. I think the best way to support this is to have an option in the top-level CMakeLists.txt file called |
I don’t quite like this approach because it would be awkward to use the library if only some of the compilation units have exceptions disabled, while other have them enabled and use throwing functions. Unless there is some cmake feature that I'm not aware of... We could also replace
I know that |
It is a huge jump to assume that because someone uses a "-fno-exceptions" dialect of C++ that they must also want a "freestanding-deleted" version of the standard library. Several major companies disable exceptions who aren't working in the embedded space.
Mixing standard C++ and a "-fno-exceptions" dialect of C++ in the same codebase is going to result in ODR violations eventually. I'm okay with this being a more awkward setup to support. The [CPP.NO_FLAG_FORKING] rule ensures that your project includes only one version of a library. This is a good practice that guarantees you won't get ODR violations due to people using SFINAE/decltype/concepts on the library's functions.
I believe replacing I think the CMake flag would be better called |
|
Ok, then I will leave this PR for the 'freestanding-delete' implementation only and address the no-exceptions in a different one |
That's fair.
I'm less certain on this. It's true that they say they need to support exceptions, but then many of the method exclusions are done because a method might throw. So I think in reality if you're going with freestanding you're also most likely going to have exceptions off. But I don't disagree with separating the two modes.
In my view the flag should BEMAN_INPLACE_VECTOR_FREESTANDING, without deleted. Yes, the net effect is to delete functions -- but FREESTANDING_DELETED implies the inverse of FREESTANDING to me -- which is HOSTED. w.r.t the NO_EXCEPTIONS -- it might be exactly the same as FREESTANDING... |
|
I am for doing |
Co-authored-by: Jeff Garland <[email protected]>
|
Moved to PR description. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Actually, I will try to merge this PR, and we can move to your separate implementation approach later on. |
Current approach is inconvenient and quite limiting. I dont think it is worth merging as we will have to undo all of the although there were some commits with general improvement unrelated to freestanding that should be cherrypicked |
Yeah I don't want to do all these testing adjustments for freestanding again, I can limit the scope of this PR to only adding testing support for freestanding delete. |
|
Actually, I will wait for your PR up and I think we need to re think the testing approach a bit... Current approach is a bit messy and unclear. |
|
Current testing structure is heavily assumed that there's only one implementation of IV, idk how we will need to update our test suite to accommodate two. |
both implementations will be derived from a single base, so we will only need to test the difference (deleted or alternative functions). I will try to make a preliminary PR until end of the week, rn a bit busy with day job. |
…_VECTOR_FREESTANDING_DELETED is on
|
@wusatosi I think there's some confusion on the meaning of freestanding. All hosted C++ compiler implementations are also freestanding implementations. The difference is that a freestanding implementation 1) may (or may not) omit certain headers, and 2) may (or may not) use "= delete" on designated functions. What this PR is doing is adding a configuration so that this library will use "= delete" on every function that could be deleted with a freestanding implementation. The , for all the functions that can be deleted under freestanding (IMO this is better style than trying to implement a more complicated macro to avoid boilerplate). You also need to do the following for your tests: Yes, GTest may not compile under some freestanding implementations. I don't think that has any bearing on this PR. |
|
@20162026 the testing bench should be ready. |
I'll agree that the freestanding flag can be flipped independent of the compiler mode. However, there's very good reason to run tests with a freestanding option -- specifically that the impl doesn't attempt to access deleted functions. I'd also argue that the cmake should in fact detect the freestanding option on the compile and automatically select the freestanding version of the impl. |
I don't think that freestanding is something we can simply test by enabling some compiler options (let alone detect with CMake), as it's not really a well-defined subset and more of a warning that the function may not exists in freestanding. As I understand it, by deleting all of the freestanding-delete functions, we're just creating a minimal implementation that must work on any platform (hosted or freestanding) that complies with the standard. IMHO, the best we can do for testing is ensure that the deleted functions are inaccessible, while the others behave exactly as they would with the hosted implementation
And yes, I do prefer the alternative approach, as it is more flexible and does not require conditional macros. |
I don't really get what you're suggesting here. But right now this PR structures tests in a non freestanding functions and freestanding only set. |
I think the test bench is setup so that both PR is testable now. Regarding which implementation to go for, I prefer #90 as well, it is significantly enough that I think having an explicit struct will be more clean. If we agree on this we can start updating this PR as a testing bench improvement PR, and strip out all the updates to the inplace_vector. |
Personally, I think it would be easier to just make a new PR than try to salvage this one... as there are just too many irrelevant changes. In addition, with the new approach there will be no cmake flag/macro or need for conditional compilation P.S. is there any Beman consensus regarding code coverage tools and etc? |
This makes sense. I can try to open another PR for this.
We have not reached a consensus yet besides using coverall. We can implement it here as another CI job though. We can track this at #82 . Should we close this PR? |
Add a freestanding build option that would remove (
= delete) all modifier, element access, and resize member functions marked withfreestanding-deletein the standard.This feature would allow more convenient usage of
inplace_vectorin a freestanding or no-exception environment (e.g. embedded field) by generating a compile-time warning instead of a runtime throw/abort.Some of the potentially throwing constructors could be kept for convenience (e.g.,
initializer_list,size,size+valueconstructors), while leaving it up to the user to ensure proper constructor usage.alternative implementation
we could have a freestanding implementation in a different namespace e.g.
beman::freestanding::inplace_vectororbeman::inplace_vector::freestanding::inplace_vector, this might allow us to avoid using macros, CMake options and any potential ODR violations.implementation
Test disabled:
Test units cleared:
TODO:
Functions out of scope:
noexcept(N == 0 || is_nothrow_move_constructible_v);
noexcept(N == 0 || (is_nothrow_move_assignable_v &&
is_nothrow_move_constructible_v));
constexpr pointer try_emplace_back(Args&&... args);
constexpr reference unchecked_emplace_back(Args&&... args);
noexcept(N == 0 || (is_nothrow_swappable_v &&
is_nothrow_move_constructible_v));
const inplace_vector& y);
operator<=>(const inplace_vector& x, const inplace_vector& y);
noexcept(N == 0 || (is_nothrow_swappable_v &&
is_nothrow_move_constructible_v))
{ x.swap(y); }