Skip to content

Conversation

@patrickroberts
Copy link
Collaborator

To me, this particular usage of a configuration header generated by CMake doesn't make sense. As a header-only library, the deployment should allow the target platform to decide which attribute the macro expands to during compilation. The header still checks #ifndef BEMAN_ANY_VIEW_NO_UNIQUE_ADDRESS in case the user wants to expand a different attribute (or none at all) with -D or #define before including the library.

Marking this as draft because I don't anticipate strong consensus on this and am fine with closing if there are good reasons not to go this route.

@nickelpro
Copy link
Member

@JeffGarland JeffGarland requested a review from camio July 6, 2025 23:00
@JeffGarland
Copy link
Member

user wants to expand a different attribute (or none at all) with -D or #define before including the library.

I think the only viable user intervention here would be no attribute. To me, for any_view this shouldn't really a user facing decision -- it's a required portability difference to support a non-compliant compiler. Not adding the attribute doesn't seem like something we need to document and have the user choose or not. Noting that the library doesn't attempt to support any compiler that doesn't have the attribute capability at all.

I also argue that this change doesn't actually violate the intent of 'flag forking'. That is, there's zero risk of down stream linking issues in this case because the library simply selects the correct option for the compiler being used. There's no cross pollination between msvc and gcc at all. And further, the change eliminates a potential packaging issue -- that is, something package with a config.hpp file for gcc will fail to compile with msvc.

We need @camio to weigh in -- this will be coming up at the monday sync tomorrow so hopefully we can resolve soon.

@JeffGarland
Copy link
Member

JeffGarland commented Jul 7, 2025

Relevant: https://discourse.bemanproject.org/t/a-haters-case-against-config-hpp/434

I've made a lengthy comment on this now -- thanks for bring that up.

@patrickroberts patrickroberts marked this pull request as ready for review September 8, 2025 16:14
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