This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement the include order checker for LLVM.
ClosedPublic

Authored by bkramer on Jul 31 2014, 7:56 AM.

Details

Summary

There are still a couple of rough edges in here but it is working fine
on LLVM and generates the same results as sort_includes.py if there are
no blank lines involved.

Diff Detail

Event Timeline

bkramer updated this revision to Diff 12064.Jul 31 2014, 7:56 AM
bkramer retitled this revision from to [clang-tidy] Implement the include order checker for LLVM..
bkramer updated this object.
bkramer added reviewers: alexfh, djasper.
bkramer added a subscriber: Unknown Object (MLST).
djasper edited edge metadata.Jul 31 2014, 8:31 AM

Just some high-level comments for now.

clang-tidy/llvm/IncludeOrderCheck.cpp
28

If we only look at includes in the main file, will we ever get any warnings about the include order in headers?

50

Maybe use translationUnitDecl() so that the empty callback isn't called so often? Probably doesn't matter, though.

Also, couldn't you do that based on PPCallbacks::FileChanged() (when it leaves the main file)?

150

The "but" seems wrong.

alexfh added inline comments.Jul 31 2014, 8:57 AM
clang-tidy/llvm/IncludeOrderCheck.cpp
27–28

It would be useful to check the include order in headers too (except for system headers). The results from unwanted headers are filtered afterwards.

54

"Category" is too vague, a more specific name would be better. Maybe "sorting priority", or "precedence"?

76

The CR newline style seems to be old enough to be useless. And without supporting it std::strchr would do the job.

101

So, this won't work for files consisting only of #includes? That's sub-optimal.

104

How about a pure-lexer approach to finding the end of includes block? Just lex everything in between #include directives until you find a non-comment token that is not a part of a preprocessor directive?

BTW, is this needed at all if you only sort includes within each block?

clang-tidy/llvm/IncludeOrderCheck.h
29 ↗(On Diff #12064)

Please move the function definition to the .cpp file.

47 ↗(On Diff #12064)

It's a bit confusing. What if the first include in a file was just an arbitrary header, which happened to be included first? What if the first include isn't special (e.g. we're analyzing a header file)?

Should we try to do filename matching?

bkramer added inline comments.Jul 31 2014, 9:10 AM
clang-tidy/llvm/IncludeOrderCheck.cpp
28

I'm not entirely sure if the strategy is to run clang-tidy on the header file to get results or if we want to also have them when running it just on the .cpp file.

But since header results are filtered by default it should be fine to remove this check.

50

There is no tranlationUnitDecl() matcher (yet). Reusing PPCallbacks is a good idea, I'll try that.

104

It should be fine to even sort clusters of #includes in the middle of the code in LLVM, but I have to double check that.

clang-tidy/llvm/IncludeOrderCheck.h
47 ↗(On Diff #12064)

I tried to do filename matching (even with some fuzzyness) and it just doesn't work for LLVM. e.g. in lib/Analysis many passes have "Passes.h" at the top which is totally unrelated to the filename :(

alexfh added inline comments.Jul 31 2014, 9:36 AM
test/clang-tidy/llvm-include-order.cpp
2

Why can't we use check_clang_tidy_fix.sh here?

bkramer added inline comments.Jul 31 2014, 9:48 AM
test/clang-tidy/llvm-include-order.cpp
2

There is currently no way to add additional flags (include path in this case) with the shell script. The other reason is that this test case produces an error (the includes don't exist) which the script would catch.

alexfh added inline comments.Jul 31 2014, 9:59 AM
test/clang-tidy/llvm-include-order.cpp
2

I see. How about adding the headers to the Inputs/ directory and adding --isystem=%S/Inputs for "system" headers? Relying on the error recovery doesn't seem correct here.
We could also forward all arguments of the script that we don't use to clang_tidy.

bkramer updated this revision to Diff 12100.Aug 1 2014, 4:38 AM
bkramer edited edge metadata.
  • Moved everything into the PPCallback. Required adding some accessors to ClangTidyCheck.
  • Removed check for "first decl", don't report errors from the main file.
  • Made test use check_clang_tidy_fix.sh. Add dummy headers and make the script forward arguments.
alexfh added inline comments.Aug 1 2014, 7:35 AM
clang-tidy/llvm/IncludeOrderCheck.cpp
49

It's wrong to do something here after the check dies. We either need to fix the order of the EndOfMainFile call (it seems to be better to call it before EndSourceFileAction in FrontendAction::EndSourceFile()) or perform the analysis in a different callback.

76

So why do we care about CR line endings? ClangFormat doesn't support them, for example, and we didn't get any requests to add support for them, afaik.

110

Does "block include sorting" correctly describe what really happens? Don't #define/#if just delimit blocks of includes?

bkramer updated this revision to Diff 12198.Aug 5 2014, 8:48 AM
  • Updated for the change that reorders the calls to EndOfMainFile in clang.
  • Limited new line checking to \n, which should be sufficient.
  • Clarified comments a bit.
alexfh accepted this revision.Aug 5 2014, 9:46 AM
alexfh edited edge metadata.

Looks good provided the comments are addressed.

clang-tidy/llvm/IncludeOrderCheck.cpp
71

Now that it is just one call, it's not particularly useful to have a separate function. Maybe inline it?

103

It probably doesn't matter much, but I don't think we need presumed locations here. Considering #line directives doesn't make much sense in this context.

142

Maybe "#includes are not sorted properly"?

This revision is now accepted and ready to land.Aug 5 2014, 9:46 AM
bkramer closed this revision.Aug 7 2014, 2:58 PM
bkramer updated this revision to Diff 12294.

Closed by commit rL215152 (authored by d0k).