Skip to content

[clang-tidy][IncludeCleaner] Fix analysis supression in presence of verbatim spellings #68185

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

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

kadircet
Copy link
Member

@kadircet kadircet commented Oct 4, 2023

No description provided.

@kadircet kadircet requested a review from VitaNuo October 4, 2023 07:41
[Resolved = (*I.Resolved).getFileEntry().tryGetRealPathName()](
const llvm::Regex &R) { return R.match(Resolved); }))
Unused.push_back(&I);
auto StdHeader = tooling::stdlib::Header::named(
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you could move this passage into the handling of standard headers in shouldIgnore. Then the below could also be simplified.

Copy link
Member Author

Choose a reason for hiding this comment

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

ATM shouldIgnore is operating on an include_cleaner::Header (for good reason, as we need to use it both for missing and unused includes). Hence we can't really go from a physical Header to a stdlib one inside shouldIgnore. We can add overloads to accept both a Header and an Include, then we can move this logic into the relevant overload, but I don't think that'll be any cleaner.

Did you have something else in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks, then what about something like (apologies for small mistakes, just typing into the comment window):

include_cleaner::Header getHeader(include_cleaner::Include I) {
    auto StdHeader = tooling::stdlib::Header::named(I.quote(), PP->getLangOpts().CPlusPlus ? tooling::stdlib::Lang::CXX :  tooling::stdlib::Lang::C);

   if (StdHeader) 
      return include_cleaner::Header(*StdHeader);
   return include_cleaner::Header(I.Resolved);
}

auto Header = getHeader(I);
if (shouldIgnore(Header))
 continue;

I'm trying to simplify the flow in the check method, as it seems like it's getting bogged down in implementation details. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that would be even better, but it would be a regression to current users. ATM this code is checking both the StdHeader and the Resolved header. In your suggestion we would only check for one or the other.

Copy link
Contributor

@VitaNuo VitaNuo left a comment

Choose a reason for hiding this comment

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

Thank you. What was the bug that triggered this? It seems like the verbatim headers were only matched if they were specified as quoted in the ignore config?

@kadircet
Copy link
Member Author

kadircet commented Oct 4, 2023

Thank you. What was the bug that triggered this? It seems like the verbatim headers were only matched if they were specified as quoted in the ignore config?

I just noticed it while trying to fix an internal issue.

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

Overall looks fine, but please update release notes for this (update current entry or even merge them together instead of adding new one). And make sure that there is a commit description for this change when committing, instead of just having only a title.

@kadircet
Copy link
Member Author

kadircet commented Oct 5, 2023

Overall looks fine, but please update release notes for this (update current entry or even merge them together instead of adding new one). And make sure that there is a commit description for this change when committing, instead of just having only a title.

done.

@kadircet kadircet merged commit 77feba5 into llvm:main Oct 5, 2023
@kadircet kadircet deleted the fix_ignore_for_verbatim branch October 5, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants