-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[Orc] Add NotifyCreated callback for LLJITBuilder #84175
[Orc] Add NotifyCreated callback for LLJITBuilder #84175
Conversation
It seems like there are no tests for |
Please note the differences to the existing callbacks
|
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.
I think it makes sense for NotifyCreated
to return Error
rather than void
so that the callback itself can fail. Otherwise this looks great -- thanks @weliveindetail!
Change the return type to allow Error results
|
||
if (impl().NotifyCreated) | ||
if (Error Err = impl().NotifyCreated(*J)) | ||
return Err; |
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.
I don't know why the premerge CI didn't fail, but everywhere else in this function is return std::move(Err);
I made the change in 5d33f71
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.
Seems like clang requires it but not gcc (which is used in premerge) nor MSVC.
You should have got a lot of buildbot emails though, didn't you?
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.
I got a lot of buildbot emails, but they also came all day for all kinds of unrelated reasons... Anyway, sorry for the mess.
This is useful to attach generators to JITDylibs or inject initial symbol definitions.