-
Notifications
You must be signed in to change notification settings - Fork 21
Change intrusive_ptr_add_ref/release signatures so ADL can find them #678
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
Change intrusive_ptr_add_ref/release signatures so ADL can find them #678
Conversation
|
What and which version of compiler did you use? Could you also provide the error messages from the compiler? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #678 +/- ##
=======================================
Coverage 28.05% 28.05%
=======================================
Files 215 215
Lines 43948 43950 +2
Branches 16805 16805
=======================================
+ Hits 12328 12331 +3
Misses 29617 29617
+ Partials 2003 2002 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
My build commands are
I attached log messages for both cases: |
|
Why those functions cannot be found by ADL? |
|
I’m not entirely sure. Here’s a brief description; I’ll investigate this tomorrow. The class Cytnx/include/backend/Storage.hpp Line 27 in 0e81a53
In Cytnx/include/backend/Scalar.hpp Lines 2514 to 2515 in 0e81a53
At that point, only a forward declaration of In the other cases, the class definitions are in the same translation unit, so this problem does not occur. |
|
I moved the forward declarations The build succeeded after this change, which supports my understanding. |
|
Maybe we also don't need to change the signature in intrusive_ptr_base.hpp? Is the final solution of this problem moving class Storage_base to a new header file? |
|
I tried building with the original function signatures and the forward declarations to the void intrusive_ptr_add_ref(intrusive_ptr_base<Storage_base> const *);
void intrusive_ptr_release(intrusive_ptr_base<Storage_base> const *);The ADL succeeded, but the type conversion failed; the error message is I believe my current modification is the smallest one that resolves the build error. As you've pointed out, a complete solution would likely involve introducing a new header file and re-organizing dependencies. |
I encountered a build failure in my environment (macOS on Apple Silicon with Homebrew).
The functions
intrusive_ptr_add_refandintrusive_ptr_releasedidn't match Boost’s expected signatures (see the reference), so the unqualified calls were not found by ADL.They now follow
Forward declarations for
Storage_baseare also added so these functions are found by ADL.This resolves the build in my environment.
The coupling the
intrusive_ptr_basecode to the concreteStorage_basetype isn't ideal.If you have a cleaner way to decouple this, I'm happy to revise.