This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Tweak diagnostic logic so suppress-in-hedaer logic works in tools too.
ClosedPublic

Authored by sammccall on Jul 13 2022, 7:27 AM.

Details

Summary

Certain idioms are ignored by -Wunused in header files only.
The current "is a header" check assumes that if headers are the main file, we're
building a PCH or a module or something. However in tools we may be parsing the
header in its own right, but still want to treat it as a header.

Fixes https://github.com/clangd/vscode-clangd/issues/360

Diff Detail

Event Timeline

sammccall created this revision.Jul 13 2022, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 7:27 AM
sammccall requested review of this revision.Jul 13 2022, 7:27 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 13 2022, 7:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sammccall updated this revision to Diff 444253.Jul 13 2022, 7:28 AM
sammccall edited the summary of this revision. (Show Details)

add bug link to description

It's possible we should *only* be checking the IsHeaderFile flag here and drop all the sourcemanager stuff, but I'm not sure of the implications - thoughts appreciated.

kadircet added inline comments.Jul 14 2022, 1:29 AM
clang/lib/Sema/SemaDecl.cpp
1866

i think we can just bail out early here when the main file is a header file (i.e. lang opts have IsHeaderFile set).

It's possible we should *only* be checking the IsHeaderFile flag here and drop all the sourcemanager stuff, but I'm not sure of the implications - thoughts appreciated.

As discussed offline this idea doesn't make sense. LangOpts tells us about the main file, so we still need to check that it's in the main file for that to be relevant.

clang/lib/Sema/SemaDecl.cpp
1866

This makes sense.

I'd rather do this as a separate commit in case we need to revert.
This one is a functional change the IsHeaderFile check to a place that's already testing for header-like contexts.
The other would be a (hopefully) NFC for performance.

(This function isn't terribly hot in the clangd profiles I have, but seems like a reasonable change to me anyway)

kadircet accepted this revision.Jul 14 2022, 3:15 AM
kadircet added inline comments.
clang/lib/Sema/SemaDecl.cpp
1866

SG. i was actually worried about the behaviour implied by the name "isMainFileLoc". i feel like both TU_Complete and IsHeaderFile check should actually happen here (or at least outside of the isMainFileLoc).

This revision is now accepted and ready to land.Jul 14 2022, 3:15 AM
nridge added a subscriber: nridge.Jul 17 2022, 12:46 AM

nit: typo in commit message ("hedaer")