This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix #55134 (regression introduced by 5da7c04)
ClosedPublic

Authored by salman-javed-nz on May 21 2022, 9:17 PM.

Details

Summary

5da7c04 introduced a regression in the NOLINT macro checking loop, replacing the
call to getImmediateExpansionRange().getBegin() with
getImmediateMacroCallerLoc(), which has similar but subtly different
behaviour.

The consequence is that NOLINTs cannot suppress diagnostics when they are
attached to a token that came from a macro argument, rather than elsewhere
in the macro expansion.

Revert to pre-patch behaviour and add test cases to cover this issue.

https://github.com/llvm/llvm-project/issues/55134

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2022, 9:17 PM
salman-javed-nz requested review of this revision.May 21 2022, 9:17 PM


This screenshot shows the code snippet from the GitHub issue, and Clang-Tidy's behaviour before and after this fix.

Thanks for preparing this revision @salman-javed-nz!

Do you think it could be worth adding a few more test cases to cover this? It turned out that this issue wasn't actually specific to multi-line macros (see this comment), so if the test case on line 96 was passing then it must not be fully/correctly testing NOLINT for single-line macros. I guess the only way to do this would be to add more checks to the nolint.cpp file, but I realise that's not a trivial change.

It turned out that this issue wasn't actually specific to multi-line macros (see this comment),

Indeed, single-line vs. multi-line was a red herring. Rather, the issue was specific to cases where the diagnostic being suppressed was attached to a token that came from a macro argument, rather than elsewhere in the macro expansion.

salman-javed-nz edited the summary of this revision. (Show Details)

Renamed test case to better explain what it is that it's testing.

The pre-merge checks are failing on the clang-format check. Not sure why it's complaining about the formatting of the lit test files - those files have never complied with the project style and have not been checked for style for as long as I remember. Has this policy changed recently?

salman-javed-nz edited the summary of this revision. (Show Details)May 21 2022, 10:55 PM
salman-javed-nz updated this revision to Diff 431223.EditedMay 22 2022, 3:12 AM

Add another test, to test the specific code sample provided in the GitHub issue (check=cppcoreguidelines-pro-type-member-init).

Thanks for preparing this revision @salman-javed-nz!

Do you think it could be worth adding a few more test cases to cover this? It turned out that this issue wasn't actually specific to multi-line macros (see this comment), so if the test case on line 96 was passing then it must not be fully/correctly testing NOLINT for single-line macros. I guess the only way to do this would be to add more checks to the nolint.cpp file, but I realise that's not a trivial change.

I added a second test case to this patch, to cover the specific check mentioned in the GitHub ticket.
This fix is check-agnostic, so I don't think we need to add even more tests than the two proposed here.

I think I've addressed what everyone has discussed in this review up to this point. Let me know what other updates I can make to this patch.

This fix is check-agnostic, so I don't think we need to add even more tests than the two proposed here.

Yes, you're right; the issue itself is check-agnostic too, I just didn't realise that when I wrote the above comment (thanks to @nridge for explaining what's really going on).

nridge accepted this revision.May 23 2022, 11:26 PM

I'm not a clang-tidy maintainer so I don't know if I have the authority to green-light this patch, but I'm fairly confident the fix is correct (it's restoring the previous usage of the SourceManager API in this loop prior to this refactor), and the tests capture the changed behaviour.

This revision is now accepted and ready to land.May 23 2022, 11:26 PM
aaron.ballman accepted this revision.May 24 2022, 5:03 AM

LGTM, though this should have a release note for the fix.

Added release note.

(Also taking the opportunity to run the patch through the pre-merge build bot one last time.)

This revision was landed with ongoing or failed builds.May 24 2022, 4:33 PM
This revision was automatically updated to reflect the committed changes.