Skip to content
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

boost::span: Add BOOST_ASSERT() to catch undefined behavior more easily #188

Closed

Conversation

SoapGentoo
Copy link

@SoapGentoo SoapGentoo force-pushed the boost-core-span-BOOST_ASSERT branch from d429122 to 274ae55 Compare January 11, 2025 15:21
@SoapGentoo SoapGentoo changed the title Add BOOST_ASSERT() to catch undefined behavior more easily boost::span: Add BOOST_ASSERT() to catch undefined behavior more easily Jan 11, 2025
@SoapGentoo SoapGentoo force-pushed the boost-core-span-BOOST_ASSERT branch from 274ae55 to 761c2b7 Compare January 11, 2025 15:55
@SoapGentoo SoapGentoo force-pushed the boost-core-span-BOOST_ASSERT branch from 761c2b7 to 04298d9 Compare January 11, 2025 17:43
@glenfe
Copy link
Member

glenfe commented Jan 17, 2025

@SoapGentoo See b0fc739 on branch feature/span. I wanted to see if not losing C++11 constexpr was possible.

I'll add more constexpr tests. The initial ones are just to cover the new asserts proposed.

@SoapGentoo
Copy link
Author

@glenfe sure, that's fine too

@SoapGentoo
Copy link
Author

@glenfe is it legal to call some form of assert() in C++11 constexpr?

@glenfe
Copy link
Member

glenfe commented Jan 17, 2025

@SoapGentoo This would be usable in C++11 constexpr:

#ifdef NDEBUG
#define BOOST_CORE_ASSERT(c) void(0)
#else
#define BOOST_CORE_ASSERT(c) ((c) ? void(0) : []{ assert(!#c); }())
#endif

Looking into other approaches too.

@glenfe
Copy link
Member

glenfe commented Jan 18, 2025

@SoapGentoo Merged 24a8174 and a subsequent commit after the tests passed on the feature branch. The idea is to now see what else breaks, if any, before adding more.

Eventually, I would just replace BOOST_CORE_ASSERT with BOOST_ASSERT after we no longer support GCC 4.x or older.

@glenfe glenfe closed this Jan 18, 2025
@SoapGentoo SoapGentoo deleted the boost-core-span-BOOST_ASSERT branch January 18, 2025 12:50
@pdimov
Copy link
Member

pdimov commented Jan 18, 2025

I'm not sure I like this that much. It foregoes the use of BOOST_ASSERT for everyone just because GCC 4.x/5 doesn't support it in constexpr, but the number of users of constexpr spans under GCC 4.x/5 -std=c++11 is zero and will remain zero.

Plus, it makes available a macro (BOOST_CORE_ASSERT) that sounds and looks like a recommended replacement for BOOST_ASSERT, and it definitely isn't one.

@Lastique
Copy link
Member

If the macro is not public, it should definitely have DETAIL or something like that in the name.

And I agree with Peter that we don't need another public assert facility.

@glenfe
Copy link
Member

glenfe commented Jan 18, 2025

No disagreement about the name. It was intended to be private. Renamed accordingly.

@pdimov
Copy link
Member

pdimov commented Jan 18, 2025

I think that this should just use BOOST_ASSERT and disable the constexpr tests for GCC 4.x and 5/c++11, as there's essentially zero demand for C++11 constexpr use of span (if you try actually writing constexpr code using spans you'll see that C++11 is completely unworkable.)

But failing that, the detail macro should fall back to BOOST_ASSERT when the compiler isn't GCC 4.x or 5.

I'm not sure whether avoiding Config here makes any practical sense, but I suppose that's a matter of taste. I'd just use BOOST_WORKAROUND(BOOST_GCC, < 60000) instead.

@glenfe
Copy link
Member

glenfe commented Jan 18, 2025

It's not just the constexpr tests that would fail on GCC 4.x it's also the regular span tests.
The other alternative I'm OK with is using BOOST_ASSERT but disabling assertions on GCC 4. i.e. 787b03e on branch https://github.com/boostorg/core/compare/feature/span

@pdimov
Copy link
Member

pdimov commented Jan 18, 2025

What I do in Boost.Array is remove the constexpr for 4.x: https://github.com/boostorg/array/blob/01983ff60439e0e0b1defdab3ca6c068be513472/include/boost/array.hpp#L118-L124

Not sure why GCC 5 is OK with that there, but isn't here.

@pdimov
Copy link
Member

pdimov commented Jan 18, 2025

Probably because it isn't tested yet. :-)

@pdimov
Copy link
Member

pdimov commented Jan 19, 2025

In Boost.Array's case, I had already written a test for the asserts

https://github.com/boostorg/array/blob/01983ff60439e0e0b1defdab3ca6c068be513472/test/array_access_test.cpp#L55

so removing the asserts for GCC 4.x actually broke it and I decided to remove the constexpr instead.

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.

4 participants