-
Notifications
You must be signed in to change notification settings - Fork 33
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
Make all public classes of CSP visible for user extension modules #365
base: main
Are you sure you want to change the base?
Conversation
78e4af3
to
5cc011e
Compare
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.
Can we also add a build / test of a user extension node and adapter as part of this
cpp/csp/core/SRMWLockFreeQueue.h
Outdated
@@ -14,7 +14,7 @@ namespace csp | |||
one of the simplest! template type is required to have an intrinsic next pointer | |||
*/ | |||
template< typename T > | |||
class alignas(CACHELINE_SIZE) SRMWLockFreeQueue | |||
class CSP_PUBLIC alignas(CACHELINE_SIZE) SRMWLockFreeQueue |
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.
Looks like some of these are missing the Platform.h
include leading to the syntax errors in compilation
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.
No, the header propagates in from csp/core/TIme.h
in this case. There's some weird behavior with the combination of alignas
and CSP_PUBLIC
, although locally it builds fine for me.
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.
ITs good practice to always include all of your deps directly, otherwise you can break if the header you rely on to transitively include stops including your dep.
As for the error, I havent seen which build failed but if its macos it could be the compiler
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.
Included all the headers btw
…cess them Signed-off-by: Adam Glustein <adam.glustein@point72.com>
5cc011e
to
49a7bfa
Compare
Closes #363