Page MenuHomePhabricator

[clang-tidy] Expand modular headers for PPCallbacks
ClosedPublic

Authored by alexfh on Mar 18 2019, 6:21 PM.

Details

Summary

Add a way to expand modular headers for PPCallbacks. Checks can opt-in for this
expansion by overriding the new registerPPCallbacks virtual method and
registering their PPCallbacks in the preprocessor created for this specific
purpose.

Use module expansion in the readability-identifier-naming check

Diff Detail

Repository
rL LLVM

Event Timeline

alexfh created this revision.Mar 18 2019, 6:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2019, 6:21 PM
alexfh updated this revision to Diff 191227.Mar 18 2019, 6:25 PM
  • Fix a typo in the comment. Rearrange paragraphs to make the comment more readable.
alexfh updated this revision to Diff 191230.Mar 18 2019, 6:49 PM
  • Fix naming.
MyDeveloperDay added a project: Restricted Project.Mar 21 2019, 3:23 AM
gribozavr added inline comments.Mar 21 2019, 6:18 AM
clang-tools-extra/clang-tidy/ClangTidy.h
156 ↗(On Diff #191230)

Please document that ModuleExpanderPP also provides events from the main file (until I read the implementation in the check, I thought the check should subscribe to both preprocessors).

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
19 ↗(On Diff #191230)

Three slashes in doc comments (everywhere in the patch).

20 ↗(On Diff #191230)

"Records that a given file entry is needed for replaying callbacks."

"addNecessaryFile"?

23 ↗(On Diff #191230)

"recordFileContent"?

24 ↗(On Diff #191230)

const & for File and ContentCache ?

38 ↗(On Diff #191230)

"remove the file from the set of necessary files..."

71 ↗(On Diff #191230)

of => off

However I don't see value in a comment that repeats the source code.

115 ↗(On Diff #191230)

Recorder->addFileToRecord(IF.getFile())

The intermediate variable doesn't add anything...

171 ↗(On Diff #191230)

What's target_callbacks_? It is only mentioned in comments.

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
9 ↗(On Diff #191230)

This looks like a documentation comment for the class itself, I think it should be moved there.

43 ↗(On Diff #191230)

"Returns the preprocessor that provides callbacks for contents of modular headers.

This preprocessor is separate from the one used by the rest of the compiler."

clang-tools-extra/test/clang-tidy/expand-modular-headers-ppcallbacks.cpp
31 ↗(On Diff #191230)

Please also add a check for a diagnostic coming from the main file.

gribozavr added inline comments.Mar 21 2019, 6:42 AM
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
32 ↗(On Diff #191230)

replays => re-runs

alexfh updated this revision to Diff 191703.Mar 21 2019, 8:22 AM
alexfh marked 15 inline comments as done.
  • Addressed review comments. Parse code to the end (seems right to do this, but no specific test that would fail otherwise).
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
24 ↗(On Diff #191230)

Done for ContentCache. The File is stored into a DenseSet, thus a pointer makes sense here.

171 ↗(On Diff #191230)

It's an artifact of an older version. Removed.

Thank you for the review! I hope I covered all the points.

gribozavr accepted this revision.Mar 22 2019, 6:10 AM
gribozavr marked an inline comment as done.
gribozavr added inline comments.
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
166 ↗(On Diff #191703)

Haha, so the test that I asked to add did catch a bug -- just not the one I expected :)

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
44 ↗(On Diff #191703)

"... callbacks for the whole translation unit, including the main file, textual headers, and modular headers."

Sorry, I wrote the comment before I fully understood what this preprocessor does.

This revision is now accepted and ready to land.Mar 22 2019, 6:10 AM
alexfh marked 3 inline comments as done.Mar 22 2019, 6:25 AM
alexfh added inline comments.
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
166 ↗(On Diff #191703)

Nope, the test passes without this. I'm not even sure this override is changing any observable behavior of PPCallbacks. I hope to find this out by running other checks on real code with modules enabled.

clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
44 ↗(On Diff #191703)

Indeed. Thanks for the text.

alexfh updated this revision to Diff 191871.Mar 22 2019, 6:31 AM
alexfh marked an inline comment as done.
  • - Update a comment according to the review comments.
This revision was automatically updated to reflect the committed changes.
alexfh marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 6:41 AM
uabelho added a subscriber: uabelho.Apr 2 2019, 1:53 AM

Hi,

I noticed that with this commit I get a whole bunch (~40) of warnings like the below when compiling with gcc 7.4:

[10/16] Building CXX object tools/clang/tools/extra/clang-tidy/utils/CMakeFiles/clangTidyUtils.dir/HeaderGuard.cpp.o
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:12:0,
                 from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:
../tools/clang/tools/extra/clang-tidy/utils/../ClangTidy.h:161:16: warning: 'virtual void clang::tidy::ClangTidyCheck::registerPPCallbacks(const clang::SourceManager&, clang::Preprocessor*, clang::Preprocessor*)' was hidden [-Woverloaded-virtual]
   virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                ^~~~~~~~~~~~~~~~~~~
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:0:
../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:35:8: warning:   by 'virtual void clang::tidy::utils::HeaderGuardCheck::registerPPCallbacks(clang::CompilerInstance&)' [-Woverloaded-virtual]
   void registerPPCallbacks(CompilerInstance &Compiler) override;
        ^~~~~~~~~~~~~~~~~~~

One of the registerPPCallbacks methods has the following comment

/// DEPRECATED: Use the other overload.
virtual void registerPPCallbacks(CompilerInstance &Compiler) {}

so I suppose it will be removed at some point?

I'm not super familiar with the warning, but there is at least one discussion about it here:
https://stackoverflow.com/questions/9995421/gcc-woverloaded-virtual-warnings