-
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
Library autoloader #308
base: main
Are you sure you want to change the base?
Library autoloader #308
Conversation
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 12. Check the log or trigger a new build to see more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
+ Coverage 72.22% 74.30% +2.07%
==========================================
Files 8 8
Lines 2963 3043 +80
==========================================
+ Hits 2140 2261 +121
+ Misses 823 782 -41
... and 2 files with indirect coverage changes
|
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 14. Check the log or trigger a new build to see more.
return nameForDlsym; | ||
} | ||
|
||
#include "../../lib/Interpreter/CppInterOp.cpp" |
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.
We probably should drop this.
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.
We use from it functions that are not visible through the .h file ie. should be available only for the tests.
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.
Okay, fair enough...
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 18. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
A couple of quick thoughts for future upstreaming: we'd want to make this compatible with out-of-process execution, and it'd be great if we could re-use parts of it for MachO auto-linking. We should consider whether |
Thanks for the feedback, @lhames. That makes sense.
I was not aware such a thing existed. @SahilPatidar perhaps that's interesting in terms of your project, too. |
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.
clang-tidy made some suggestions
0686324
to
c1dd45f
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.
clang-tidy made some suggestions
} | ||
|
||
void setEnabled(bool enabled) { | ||
Enabled = enabled; |
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.
warning: method 'isEnabled' can be made const [readability-make-member-function-const]
Enabled = enabled; | |
bool isEnabled() const { |
} | ||
|
||
void materialize(std::unique_ptr<llvm::orc::MaterializationResponsibility> R) override { | ||
if (!sAutoSG || !sAutoSG->isEnabled()) { |
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.
warning: function 'getName' should be marked [[nodiscard]] [modernize-use-nodiscard]
if (!sAutoSG || !sAutoSG->isEnabled()) { | |
[[nodiscard]] StringRef getName() const override { |
c1dd45f
to
a04706e
Compare
@aaronj0, do you have a clue why cppyy fails? |
cppyy does not fail, InterOp does not build on any of the jobs. This comes back to the point that CppInterOp is only built on the jobs tagged with "cppyy" not the first stage that only builds llvm. On a preliminary look, it seems like on llvm19 we get:
and on 18 and 17, these "undefined reference to They also fail in some of the emscripten builds but I am more interested in the fact that they pass on llvm 17 and 18 with clang-repl on all platforms |
Co-authored-by: Vassil Vassilev <[email protected]>
a04706e
to
876169a
Compare
No description provided.