Page MenuHomePhabricator

[clang-tidy] Add a generic header guard checker + LLVM implementation.
ClosedPublic

Authored by bkramer on Aug 12 2014, 7:55 AM.

Details

Summary

The implementation is split into a generic part and a LLVM-specific part.
Other codebases can implement it with their own style. The specific features
supported are:

  • Verification (and fixing) of header guards against a style based on the file path
  • Automatic insertion of header guards for headers that are missing them
  • A warning when the header guard doesn't enable our fancy header guard optimization

(e.g. when there's an include preceeding the guard)

  • Automatic insertion of a comment with the guard name after #endif.

For the LLVM style we disable #endif comments for now, they're not very common
in the codebase. We also only flag headers in the include directories, there
doesn't seem to be a common style outside.

Diff Detail

Repository
rL LLVM

Event Timeline

bkramer updated this revision to Diff 12400.Aug 12 2014, 7:55 AM
bkramer retitled this revision from to [clang-tidy] Add a generic header guard checker + LLVM implementation..
bkramer updated this object.
bkramer added reviewers: alexfh, klimek, djasper.
bkramer added a subscriber: Unknown Object (MLST).
klimek added inline comments.Aug 13 2014, 2:45 AM
clang-tidy/llvm/HeaderGuardCheck.cpp
16 ↗(On Diff #12400)

Is it intentional that this finds lib/internal/Bincludes.h?

clang-tidy/llvm/HeaderGuardCheck.h
21 ↗(On Diff #12400)

You might want to run the override clang-tidy checker :P

clang-tidy/utils/HeaderGuard.cpp
20–21 ↗(On Diff #12400)

Perhaps add a FIXME to put this somewhere more central.

29 ↗(On Diff #12400)

Do we want an error for something like cleanPath("../../../../some/path")?

71–72 ↗(On Diff #12400)

s/Defines/defined/; s/Name/name/

103–126 ↗(On Diff #12400)

Pull this into an extra method.

104–105 ↗(On Diff #12400)

s/ifndef/Ifndef/

110–111 ↗(On Diff #12400)

s/compute/compute, /

131–145 ↗(On Diff #12400)

Pull this into an extra method.

148–189 ↗(On Diff #12400)

Pull this into an extra method.

165 ↗(On Diff #12400)

s/TODO/FIXME/

170 ↗(On Diff #12400)

Any reason this an CPPVarUnder are not declared next to each other?

clang-tidy/utils/HeaderGuard.h
23 ↗(On Diff #12400)

I think this looks much nicer in doxygen if it starts with:
"Returns true if..."
But I can see how you might want it differently as it's intended to be implemented by others. Still, I'd at least start sentences with a capital letter.

27–28 ↗(On Diff #12400)

Can't parse the sentence.

unittests/clang-tidy/LLVMModuleTest.cpp
94–101 ↗(On Diff #12400)

I somehow don't believe these are enough tests :)

bkramer updated this revision to Diff 12440.Aug 13 2014, 3:35 AM

Addressed review comments.

alexfh added inline comments.Aug 13 2014, 4:26 AM
clang-tidy/utils/HeaderGuard.cpp
126 ↗(On Diff #12440)

This class is stateful. AFAIU, it's reused when a tool is run with several translation units (clang-tidy x.cpp y.cpp z.cpp), so it should clean its state here.

klimek edited edge metadata.Aug 13 2014, 4:37 AM

I still doubt we have enough tests. Otherwise LG.

clang-tidy/utils/HeaderGuard.cpp
173 ↗(On Diff #12440)

s/emits/Emits/

clang-tidy/utils/HeaderGuard.h
30–32 ↗(On Diff #12440)

:gq

bkramer updated this revision to Diff 12442.Aug 13 2014, 6:07 AM
bkramer edited edge metadata.
  • Clean PPCallbacks state after leaving file.
  • Comment fixes.
  • Also apply the style to header files outside of include/
alexfh accepted this revision.Aug 13 2014, 6:45 AM
alexfh edited edge metadata.

LG

clang-tidy/llvm/HeaderGuardCheck.cpp
21 ↗(On Diff #12442)

Can we use the location of the compilation database to make project-relative paths? We can easily pass compilation database to the check via ClangTidyContext, if need be. We should consider this, but it shouldn't block this patch, though.

clang-tidy/llvm/HeaderGuardCheck.h
21 ↗(On Diff #12442)

Why false? Aren't #endif comments used in LLVM?

24 ↗(On Diff #12442)

Do you need to provide a default argument in the overridden method? Are you using this specific method with one argument?

This revision is now accepted and ready to land.Aug 13 2014, 6:45 AM
bkramer added inline comments.Aug 13 2014, 7:01 AM
clang-tidy/llvm/HeaderGuardCheck.cpp
21 ↗(On Diff #12442)

That's probably the right thing to do in the long term. Using absolute paths is a hack and might break if the user has a weird setup. We will get FileEntries with absolute paths from time to time (e.g. #include "./foo.h" creates them), so canonicalizing them with the compilation database seems like a nice way around this problem. It would break using without a database around (header files are often missing in the database), not sure what to do about that.

clang-tidy/llvm/HeaderGuardCheck.h
21 ↗(On Diff #12442)

Some files do, the majority of headers does not, so I disabled them for now.

24 ↗(On Diff #12442)

I am calling it, but it should only be there in the base class. This was copy and paste failing.

bkramer closed this revision.Aug 13 2014, 7:07 AM
bkramer updated this revision to Diff 12444.

Closed by commit rL215548 (authored by d0k).