Skip to content

Conversation

@alexreinking
Copy link
Member

@alexreinking alexreinking commented Aug 20, 2025

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:

#define _halide_internal_diagnostic(condition, type, condition_string)  \
    /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \
    if (!(condition)) for (type _err; _err; _err.issue()) _err.init(__FILE__, __FUNCTION__, __LINE__, condition_string)

The bare if and for statements guard _err.init(...), which returns itself for the sake of << chaining. The if condition prevents evaluation, the for loop 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, the operator bool returns false after issue() has been called.

@alexreinking alexreinking requested a review from abadams August 20, 2025 19:41
@alexreinking
Copy link
Member Author

Looks like I broke correctness_custom_error_reporter.

@mcourteaux
Copy link
Contributor

adams2019_test_apps_autoscheduler failing is new. That's not the case on any of the previous builds, AFAI can remember.

@alexreinking
Copy link
Member Author

alexreinking commented Aug 21, 2025

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

@alexreinking
Copy link
Member Author

A fun blog post from JeanHeyd Meneide on the defer TS for C2y and the destructor issue we're observing:

There are also some edge cases where defer is actually better than C++, as mentioned in the rationale of the proposal. For example, exceptions butt up against the very strict noexcept rule for destructors (especially since its not just a rule, but required for standard library objects). This means that using RAII to model defer becomes painful when you intentionally want to use defer⸺or scope_guard⸺as an exception-detection mechanism and a transactional rollback feature.

https://thephd.dev/c2y-the-defer-technical-specification-its-time-go-go-go#there-are-also-some-edge-cases-where-defer-is-actually-better-th

Copy link
Contributor

@mcourteaux mcourteaux left a 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;
Copy link
Contributor

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 inheriting issue(). See the nifty for-loop trick at the _halide_internal_diagnostic macro below.

Copy link
Member Author

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
Copy link
Contributor

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?

Copy link
Member Author

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

@alexreinking
Copy link
Member Author

Failures are unrelated

@alexreinking alexreinking merged commit 7ac0f7e into main Aug 22, 2025
11 of 19 checks passed
@alexreinking alexreinking deleted the alexreinking/error-macros branch August 22, 2025 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants