-
Notifications
You must be signed in to change notification settings - Fork 24
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
TracePluginProperty fixes #420
Conversation
This commit results from changes that were made in the effort to eliminate warnings. The changes were good, but the problem that I have is that I wish for everything that will be used by code outside of the runtime to be in the trace/api directory.
The fixmes in the description of this PR seem things that need to be addressed on the Lingua Franca end, are they not? |
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 great! Thanks, @erlingrj.
I am going to double check why this was working in other projects.
ons. 8. mai 2024 kl. 22:13 skrev Marten Lohstroh ***@***.***>:
… ***@***.**** approved this pull request.
Looks great! Thanks, @erlingrj <https://github.com/erlingrj>.
—
Reply to this email directly, view it on GitHub
<#420 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFBXI4XMG6INLAVWAZQV5Y3ZBKBQDAVCNFSM6AAAAABHLF7FH6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBWGU2TINRXGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
LGTM
When trying to compile a simple example with an external trace plugin I got linker errors. It seems that we need to link with
dl
. I am not exactly sure why it has worked before without specifying this though.