Fix name conflict & Support cmake install#97
Conversation
|
I defer reviewing this PR to original author of CMake files @RedSkittleFox , please. |
| INTERFACE | ||
| ${CMAKE_CURRENT_SOURCE_DIR} | ||
| ) No newline at end of file | ||
| add_library(atomic_queue::atomic_queue ALIAS atomic_queue) |
There was a problem hiding this comment.
@max0x7ba
Changing the name from max0x7ba::atomic_queue to atomic_queue::atomic_queue is somewhat of a breaking change. Perhaps it would be sensible to make a new release because of that?
There was a problem hiding this comment.
I agree, we need make a new release after merge this.
I changed the namespace because namespaces sharing the same name as the project are more common.
For example glm::glm, nanobench::nanobench, minifb::minifb, yoga::yogacore and etc.
Since this project previously did not export CMake config, this is an ideal opportunity to change the namespace.
There was a problem hiding this comment.
And also, after make a new release, I will submit a new pr to add cmake usage about FetchContent.
RedSkittleFox
left a comment
There was a problem hiding this comment.
I've looked at both this MR and microsoft/vcpkg#50329 and tested this one locally. Both look good.
I've left two comments for the consideration. Otherwise this is ready to be merged.
If you decide to implement further changes per my suggestions I'll rereview this again.
| ${CMAKE_CURRENT_SOURCE_DIR}/atomic_queue/spinlock.h | ||
| include(GNUInstallDirs) | ||
|
|
||
| add_library(atomic_queue INTERFACE) |
There was a problem hiding this comment.
This name is very generic and might potentially conflict with some other package in the vcpkg ecosystem, perhaps it might be a good time to change this internal library name from atomic_queue to something like max0x7ba_atomic_queue and then alias that to either max0x7ba::atomic_queue or atomic_queue::atomic_queue?
There was a problem hiding this comment.
IMO changing the port name of vcpkg is not a good idea, as it would break downstream. As for naming conflicts, that is an issue future ports should consider, I would like to keep the current port name.
Well, I read a CMake book more than a decade ago, but never wrote a line of CMake since then. If you could review changes to CMake code you authored, dear @RedSkittleFox, I would click the merge button for you when you are happy with the changes 🤷🏼♂️💯😁 |
|
@max0x7ba I found that when I use this in a mm file, it has name conflict with |
Hmm, what is mm file and why does it break |
Objective-C++ (ObjC++) is an extension of the Objective-C programming language that allows seamless integration of C++ code and features within Objective-C programs. Objective-C is a core language for Apple platforms (like iOS and macOS), built on C with object-oriented extensions. ObjC++ further extends it to incorporate C++ elements. |
max0x7ba
left a comment
There was a problem hiding this comment.
Well, this library adheres to and complies with the C++14 language standard.
In C++, nil is not a reserved keyword, and is available for use as an identifier of a class, function or object, in all namespaces without restrictions.
It could also be a preprocessor macro, breaking nil identifiers in any and all namespaces. Just like Microsoft's C headers define macros min and max, breaking std::min and std::max. That's the error of Microsoft's C header files defining macros with names reserved for user code, and, hence, breaking any perfectly well-formed C++ code by the misfortune of having to include Windows C headers. Microsoft NOMINMAX macro was introduced for Windows C headers to comply with the C++ standard by stopping breaking std::min and std::max.
In your case, the well-formed C++ standard compliant headers get broken when used by applications incompatible with the C++ standard.
If you'd like to use C++ headers in incompatible with C++ standard applications and environments, that's a problem of your own making. You cannot possibly ask of 3rd-party libraries to bend over and adjust to arbitrary constraints and limitations of yours or anyone else's incompatible environments. And without associated unit-tests your changes will be broken by some later change without anyone noticing.
You should rather adjust your environment and tools to comply with the language of libraries you'd like to use. That's what standard compliance exists for.
Objective-C++ ... allows seamless integration of C++ code and features within Objective-C programs...
Objective-C++ fails to deliver its "seamless integration of C++ code and features" feature, in this particular case. You should file a bug report against proprietary Objective-C++ tool breaking C++ header files.
Does that sound reasonable to you?
IMO, it is not. I understand the Therefore, I still recommend changing |
This library is designed for C++ language. Objective-C++ is not C++, is that right?
Compatibility of Apple's proprietary Objective-C++ with the C++ standard could be essential for Apple. C++ not being compatible with Objective-C++ is Objective-C++'s problem. Is it not?
And what does this C++ library have to do with your Apple's Objective-C++ environment?
We cannot support Apple's objc++ because:
Undefine That is as small modification as it ever gets.
Are you going to recommend renaming to every C++ library that Apple's Objective-C++ breaks? |
|
@xiaozhuai the cmake and vcpkg changes were still valid contributions |
PRs with one change are the best practice. Easy to review, assess and accept. PRs with more than one change are normally rejected. They must be split into multiple one-change PRs in order to be considered review-worthy. Piling more changes into one PR while it is being reviewed is a little bit worse than submitting a PR with more than one change. |
|
I have no interest in debating the naming conflict surrounding
The answer is |
I agree : ) |
And these are not true as I know. |
|
Finally, I'd like to give another example: |
It's actually located in the namespace
It's not really abandoning, this was never designed to work with it in the first place. AFAIK you could create a thin wrapper, compile it and then use that in objective-c++. This way you wouldn't be exposing the nil identifier to objective-c++ sources. I don't think changing the name from |
Me neither. That's Apple's Objective-C++ problem to solve for its users, who pay Apple for the privilege of using Apple's Objective-C++.
This library was never intended to be compatible with anything else than C++14. It never had Objective-C++ compatibility to abandon now, to begin with. Apple's Objective-C++ is what abandons C++ compatibility, making its users suffer. Having said that, your CMake changes are most welcome. Your CMake changes don't need to suffer because of Apple's Objective-C++ abandoning compatibility with the C++ standard. |
I know that, we all know there are many workarounds.
I completely agree. Then why not?
Sure, I should have done that. But I cut corners. If the author requests it, I'm willing to do it. |
This is not true. It's a superset of c++. You also cited the example of min/max in msvc. It's common for our code to fail on certain platforms or in specific environments. We patch it to make it work on more platforms—we call this porting, am I right? If only we could ensure that all environments were consistent, the world would be perfect. |
Dear Weihang Ding, Would you be so kind as disentangle your uncontroversially valuable and most desirable CMake changes from any other changes, in order to have your CMake changes merged and, hence, let other people benefit from the fruits of your labour as soon as possible, please? You can submit any other changes of yours as separate PRs, so that we can bicker out disagreements on a per-change basis without blocking any other changes of yours? If you decide otherwise, I won't get offended. And you're welcome to change your mind any time. Maxim |
This reverts commit 89cc5ee.
Dear Maxim, My pleasure. Let's set aside our differences for now. Have a nice day. Weihang |
|
No complaints from me any more, I am happy. @RedSkittleFox Give me green light to merge, when you are happy with the changes, please. |
CMake install added.
CMake install added.
See also microsoft/vcpkg#50329
Support following cmake usage