Skip to content

Conversation

@20162026
Copy link
Collaborator

@20162026 20162026 commented May 11, 2025

add support for builds with exceptions disabled #72

@20162026
Copy link
Collaborator Author

20162026 commented May 11, 2025

  • We can add the BEMAN_INPLACE_VECTOR_NO_EXCEPTIONS CMake option and generate config.hpp, but I would like to keep defined(__cpp_exceptions) as it's the industry-standard way to handle noexception build.

  • current test implementation is ugly, but I dont know how to improve it while keeping gtest

@20162026 20162026 mentioned this pull request May 12, 2025
66 tasks
@wusatosi
Copy link
Member

Thanks for the contribution as always, I will review tomorrow.

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.

See comments

@wusatosi
Copy link
Member

We can add the BEMAN_INPLACE_VECTOR_NO_EXCEPTIONS CMake option and generate config.hpp, but I would like to keep defined(__cpp_exceptions) as it's the industry-standard way to handle noexception build.

This does make sense to me, I don't know if we want to go with this, we have a meeting agenda on discussion on the config.hpp approach of doing things vs industry standard direct macro detection, I will bring this up.

@wusatosi
Copy link
Member

Hey @20162026 , we will bring up some feedback against CPP.NO_FLAG_FORKING at the weekly sync the coming Monday, I think you have some valuable experience interacting and developing with this rule, would you mind writing a short feedback here so I can forward it to our meeting agenda?

@20162026
Copy link
Collaborator Author

@wusatosi some arguments I could think of right now, mostly regarding this and header only libs in general:

  1. I don't think that ODR violation prevention measures should be enforced for header only libraries, as it is up to the user how the compilation units that are using the library should behave.
  2. For header only (especially single header) libraries, it is inconvenient to have cmake generated config files, as the end user often just wants to drag and drop the headers or use a different build system, and would be forced to manually generate the config file
  3. This is more of a unique case and can most likely be avoided by splitting the freestanding impl into a different namespace. However, I have a project where I would like to use both HOSTED and FREESTANDING-DELETED versions of inplace_vector. If we used a preprocessor define to distinguish between them and enforce it at top level, it would require multiple instances of the same library.
  4. From personal observation, it is common practice for all of the standard libraries to FLAG FORK

@wusatosi
Copy link
Member

Thanks for the comments, will forward.

@wusatosi
Copy link
Member

oh also, how do we refer to you? : )

@20162026
Copy link
Collaborator Author

@wusatosi my name is Edgar if that's what you are asking.

Also, PR should be more or less ready, besides the NO_FLAG_FORKING compliance

@20162026 20162026 marked this pull request as ready for review May 17, 2025 20:31
@20162026 20162026 requested review from a team and RaduNichita May 17, 2025 20:31
@wusatosi
Copy link
Member

We can add the BEMAN_INPLACE_VECTOR_NO_EXCEPTIONS CMake option and generate config.hpp, but I would like to keep defined(__cpp_exceptions) as it's the industry-standard way to handle noexception build.

Do you think the CMake option that has its default value based on if __cpp_exceptions is working would be acceptable?

I can set this up directly on this PR, would you mind that?

@20162026
Copy link
Collaborator Author

20162026 commented May 18, 2025

Do you think the CMake option that has its default value based on if __cpp_exceptions is working would be acceptable?

@wusatosi do you mean checking it in the config.hpp or in CMake itself using something like check_cxx_source_compiles ? I have pushed an example of how I would imagine it.

I can set this up directly on this PR, would you mind that?

Feel free to push any changes you see fit.
PS. there's no need to ask, I’d just uncheck the Allow edits by maintainers flag on GitHub otherwise

@wusatosi
Copy link
Member

Feel free to push any changes you see fit.
PS. there's no need to ask, I’d just uncheck the Allow edits by maintainers flag on GitHub otherwise

Noted, I usually find it rude for people to push directly on my branch, so I always ask HAHA.

@20162026 20162026 requested a review from wusatosi May 19, 2025 21:42
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.

Thank you so much for all the work!

I have one minor question but besides that everything looks great.

Comment on lines +55 to +58
target_compile_options(
beman.inplace_vector.tests.noexceptions
PRIVATE -fno-exceptions
)
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it ensures that there are no throws in the compiled code. Without it, EXPECT_DEATH could still pass if an unhandled exception was thrown.

Although, I'm now not sure if we need the compiler check. It allows running the tests on compilers that don't support or have a different syntax for disabling exceptions (e.g. MSVC). But right now, CI only includes Clang and GCC, and maybe it would be better to error out instead of silently skipping tests on other compilers. On the other hand, it doesn't matter much, as the implementation should be compiler agnostic as long as there is abort()

        CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
        OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU"

@wusatosi wusatosi merged commit aaa789e into bemanproject:main May 20, 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.

2 participants