-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use a new macro trick to avoid throwing in destructors. #8774
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
Conversation
|
Looks like I broke |
|
adams2019_test_apps_autoscheduler failing is new. That's not the case on any of the previous builds, AFAI can remember. |
Hmm, that's a TIMEOUT flake. I ran it manually and it took something like 18 minutes to complete, but it did pass. I don't know how this PR could be responsible for that. It runs in 90 on my MacBook, which is about how long it always took. Also it's on the ASAN branch, so being slow is no surprise. It timed-out in this PR too: #8700 |
|
A fun blog post from JeanHeyd Meneide on the
|
mcourteaux
left a comment
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.
Got two questions, and one suggestion for a comment to clarify something. Otherwise LGTM.
src/Error.h
Outdated
| if (!msg.str().empty() && msg.str().back() != '\n') { | ||
| msg << "\n"; | ||
| } | ||
| issued = true; |
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.
The method finalize_message() set issued to true? The method issue() doesn't? This is confusing. Is there better naming possible? Looking around, I can tell now that issue() calls finalize_message(), and you want to assure that this boolean-setting step is always done for all the inheriting classes. Not sure how I would improve this. Perhaps just add a comment in finalize_message() that says:
Sets issued because every
finalize_message()is called by all inheritingissue(). See the nifty for-loop trick at the_halide_internal_diagnosticmacro below.
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.
If issue did, it happen in two places 🫠
| #define user_assert(c) _halide_internal_assertion(c, Halide::CompileError) | ||
| #define internal_assert(c) _halide_internal_assertion(c, Halide::InternalError) | ||
|
|
||
| #endif |
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.
If this file is gone from the autoschedulers, then where do the error/warning macros come from? Are they exposed in Halide.h? I thought they weren't?
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.
They are if you define HALIDE_KEEP_MACROS, which happens in HalidePlugin.h
|
Failures are unrelated |
As discovered in #8767, throwing in destructors is a bad idea. We can avoid doing this in our error handling macros using a nifty for-loop trick in the macro.
The trick appears here:
The bare
ifandforstatements guard_err.init(...), which returns itself for the sake of<<chaining. Theifcondition prevents evaluation, theforloop issues the warning or error at the end of "each" iteration. In reality, there is only one iteration in any case: for errors,issue()is marked[[noreturn]]and for warnings, theoperator boolreturns false afterissue()has been called.