This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files
AbandonedPublic

Authored by carlosgalvezp on Nov 14 2021, 9:41 AM.

Details

Summary

Currently, ClangTidyDiagnosticConsumer defines 2 additional
classes: ClangTidyContext and ClangTidyError. A reader does
not expect them to be there, and a file needing only one
of them needs to include the heavy ClangTidyDiangosticConsumer.h,
increasing compilation time.

Refactor by moving these classes into their own set of .h/.cpp files.
Besides readability, compilation time is also improved.
Adjust includes accordingly.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Nov 14 2021, 9:41 AM
carlosgalvezp requested review of this revision.Nov 14 2021, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2021, 9:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/ClangTidyContext.cpp
41

Shouldn't namespaces be used instead?

clang-tools-extra/clang-tidy/ClangTidyError.cpp
19

Shouldn't namespaces be used instead?

I agree with the comments, but I didn't want to touch any code other than moving things around, since it's hard to see the changes in the diff otherwise. I was also unsure if this was "LLVM convention" or a mistake. I'm happy to fix in a separate patch if that's OK?

What is the motivation behind the change? Can we be sure that these classes that were moved out weren't details to the context itself, and are understandable, usable, and can be relied upon without the context object?


I agree with the comments, but I didn't want to touch any code other than moving things around, since it's hard to see the changes in the diff otherwise. I was also unsure if this was "LLVM convention" or a mistake. I'm happy to fix in a separate patch if that's OK?

I think you can fix it now, LLVM style doesn't indent stuff that's within a namespace, so it won't change the meaningful part of the diff visually.

clang-tools-extra/clang-tidy/ClangTidyContext.cpp
41–42

In general, you'd also want to make it more explicit by saying

using namespace clang;
using namespace clang::tidy;

and not rely on precisely one tidy being visible from the previous set of available declarations.

clang-tools-extra/clang-tidy/ClangTidyError.h
36–37

Is this formatted properly? I thought the appropriate closing comment is // namespace blah, without the end. 😲

What is the motivation behind the change?

The motivation is cleaning the file to make it easier to find things. As a n00b to clang-tidy I find it a bit hard to navigate the code and find the different classes - they are not where I expect them to be. The ClangTidyContext is a core class used in other places than just DiagnosticConsumer, so I think it makes sense to keep it on it's own file so it's easy to find. For example:

ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context);

It's unintuitive to have to import the DiagnosticConsumer to get the Context

In general I think it's a good principle to keep a 1:1 mapping between public classes and files. Naturally, internal helper classes can be kept in the same file as the "main" class. This brings build-time improvements as well but I'd say they are negligible.

Let me know if you agree, otherwise I can of course drop this :)

carlosgalvezp marked 4 inline comments as done.
  • Rebased.
  • Addressed comments.
carlosgalvezp abandoned this revision.Jan 25 2022, 9:41 AM