Page MenuHomePhabricator

[clang-tidy] Fix PR35824
ClosedPublic

Authored by xazax.hun on Apr 24 2018, 12:19 PM.

Details

Summary

This approach will also introduce false negatives.
A better approach would be to check if the null statement is the result of folding an if constexpr.
The current AST API does not expose this information.
A better fix might require AST patches on the clang side.
The fix is proposed here: https://reviews.llvm.org/D46234

Richard suggested to replace null stmts with nulls, but I have no time to deal with all the fallouts for now. If someone is willing to pick that up feel free to, but until then it might be a good idea to land this quick fix.

Diff Detail

Event Timeline

xazax.hun created this revision.Apr 24 2018, 12:19 PM

Which solution do you prefer?

If I understand the issue properly: both. :-)

Having the AST track information that's been folded away is still useful -- some users are using the AST for purposes other than codegen, and the fact that a construct has been folded away is good to know about while still retaining as much AST fidelity as possible.

On the other hand, from an AST matcher perspective, I think it's natural for users to write ifStmt(isConstexpr()) and so that seems like a useful extension to the matcher. Further, it is extensible if the committee adds other constexpr foo statements.

As for which solution gets used by this check to fix the PR, I don't have a strong opinion at this time (currently at WG14 meetings and a bit distracted).

What is the status of the PR?

What is the status of the PR?

I believe xazax doesnt have time to work further, you can commandeer if you want :)

xazax.hun updated this revision to Diff 231078.Tue, Nov 26, 8:27 AM
  • Use matcher.
JonasToth accepted this revision.EditedWed, Nov 27, 9:46 AM

Maybe a short notice in the release notes wouldn't hurt. Otherwise LGTM

*EDIT*: Aaron commented as well. Plz let him react before committing :)

This revision is now accepted and ready to land.Wed, Nov 27, 9:46 AM
xazax.hun added a comment.EditedWed, Nov 27, 10:12 AM

Thanks!

Updated context for this patch:
A superior fix would be to follow through with the approach suggested by Richard in https://reviews.llvm.org/D46234 . However, since I do not have time to finish that and there are people complaining in the PR, I think it is better to land this.

xazax.hun edited the summary of this revision. (Show Details)Wed, Nov 27, 10:14 AM
alexfh accepted this revision.Wed, Nov 27, 10:39 AM

This approach will also introduce false negatives.

Could you add a test showing this with a FIXME comment?

A better approach would be to check if the null statement is the result of folding an if constexpr.
The current AST API does not expose this information.
A better fix might require AST patches on the clang side.
The fix is proposed here: https://reviews.llvm.org/D46234

Richard suggested to replace null stmts with nulls, but I have no time to deal with all the fallouts for now. If someone is willing to pick that up feel free to, but until then it might be a good idea to land this quick fix.

Could you add a FIXME comment to this effect?

Otherwise LG.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 27, 11:13 AM