Skip to content
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

[hannk] Pacify clang-tidy #6412

Merged
merged 4 commits into from
Nov 12, 2021
Merged

[hannk] Pacify clang-tidy #6412

merged 4 commits into from
Nov 12, 2021

Conversation

steven-johnson
Copy link
Contributor

No description provided.

We must use use_global_gc = false to work properly with the JIT
This reverts commit 9ed07a7.
return op->mutate_impl(m, std::move(op));
// clang-tidy will complain if we just do `op->mutate_impl(m, move(op))`
// because the order of evaluation between the move and the invocation is
// undefined; while that's true, we know that op will remain valid throughout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be more than clang-tidy just being fussy, I think it really is sketchy. But the code looks fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can always go revisit this an implement some other sort of dispatch approach. (Or scrap this mess entirely...) But as a practical matter, it's not clear to me how the compiler could generate misbehaving code here: the unique_ptr needs to keep the pointer alive and valid throughout the life of the call. Since UB makes all things possible, sure, the compiler could legally do anything here, I guess...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the code as written now (after this PR) is safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a revision that should be genuinely safe, PTAL

@steven-johnson steven-johnson merged commit 16fa3ce into master Nov 12, 2021
@steven-johnson steven-johnson deleted the srj/hannk-tidy branch November 12, 2021 23:17
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.

2 participants