-
Notifications
You must be signed in to change notification settings - Fork 11
Allow using library without cmake generated files #94
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
JeffGarland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree completely with this direction. The has_include logic means the config file can be present if desired, but this empowers users to simply copy the header and configure the library however they desire. So I think it achieves @camio's objective as well as mine w.r.t. users that what a single include that they drop in their project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think we should split the library out into different smaller header files for maintainability in the near future.
As I mentioned before, single header only is just a coincidence than a design, this is the opposite of the refactor we want to do. Additions like this will only add more confusion.
this empowers users to simply copy the header and configure the library however they desire
Header only deployment is not supported, this is empowering undefined behavior. The supported way is to use CMake, if you configure the include via CMake, it's going to generate the correct config file for you.
There are also couple points of customization we want to expose as build config in the future, such as using trivial unions, fix size data type to test for #53 . Keeping in check of the two sources of config is confusing and potentially error prone.
I understand @JeffGarland doesn't like David's approach, that's perfectly fine. But I don't think this is a good alternative, if an alternative at all, since I don't have a strong preference on the alternative, I think we should just follow the recommendation in the standard, which is the current status quo. Again this has been previously discussed multiple times in the sync, plus this seems to be something really hard to get right considering the repeated back and forth didn't result in any change made in the standard.
If we are for changing the way we approach the no forking rule, we should go do this in exemplar/ add this as a official alternative in the standard.
Ok sure, 'single header is meaningless. Let's consider arguably one of the most used software packages on the planet -- sqlite. Go see -- single header. Sure, you can make it into a couple headers and it'll still be fine -- but it's clear -- nothing in this library needs a .a or .so file.
So what, it's still fine.
There's nothing in this PR that 'goes against the sync' discussion.
What @20162026 has proposed follows th no forking rule, but also allows a user to take a header without the cmake -- it solves both problems. |
|
Just to clarify, I'm not against splitting the library into multiple headers (as long as they are part of src and not autogenerated), and this PR does not go against current Beman rules. It's just that it's awkward to be forced to use CMake for a header only library, as it is quite common to simply drag&drop or P.S. technically this is also the root couse of current issues with compiler explorer, becouse CE expects for regular header only lib to be just headers and not headers + cmake generated files, and if being pedantic, one could argue that this is a violation of CORE.INDUSTRY_STANDARD... |
|
At @20162026 you should go ahead and merge this - godbolt has been broken for too long. I agree with you that this doesn't violate the forking rule in any way -- this just fixes the case where the generated file isn't present -- which is in godbolt and for any user that has just dropped the header into their project. I believe the code your change allows for the correct handling and that this should be added to the standard as a clarification -- I plan to have scope library use the same pattern. If you're not comfortable with the override I'll do it, but we need PR's to not sit for long periods... |
|
@wusatosi are you still against this change? because right now your review is blocking the merge |
Improve user experience and usability of the library by removing library dependency on cmake generated config file.
This PR is just a suggestion, but I dont think that enforcing cmake generated files on a interface only library is a good think, especially for something as small as inplace vector.