njames93 added a comment.
This is a great check that I've been meaning to work on for ages but never had
time.
It mostly looks good but there are a few issues
I feel there is another issue that this can be very spammy with
diagnostics(though clang-tidy is likely suppressing the duplicated ones).
But maybe when we emit a warning for a specified include, we could put it in a
set to not warn on that include again
================
Comment at:
clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:106-110
+ if (History.size() == 2U) {
+ Check.diag(Loc, "direct self-inclusion of header file '%0'")
+ << History.back();
+ return;
+ }
----------------
How does this handle this case where an include includes a file that self
includes itself.
```lang=c++
// Foo.hpp
#pragma once
#include "Foo.hpp"
// Bar.hpp
#pragma once
#include Foo.hpp
// Main.cpp
#include "Bar.hpp"
```
Where will the warning be emitted, it should appear in `Foo.hpp` but the logic
here says it won't and would just be reported as a circular dependency below.
Can you make sure the warning is emitted in `Foo.hpp` and add test cases that
test for this, The current tests just check include a file that self references
itself with no intermediate files.
================
Comment at:
clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:112-122
+ std::string IncludePath;
+ for (const llvm::StringRef &Name : History) {
+ IncludePath += '\'';
+ IncludePath += Name;
+ IncludePath += "' -> ";
+ }
+
----------------
Rather than just printing out the include path, can you emit each include
location as a note.
`OtherInclude.h:5:1: note: 'ThisInclude.h' included here`
You can pass `DiagnosticLevel::Note` as the 3rd parameter to `Check::diag` to
emit a note. Just have to make sure you emit the note after you emit the
warning.
Notes have better support for front end diagnostics parsing.
================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:134
+private:
+ SmallVector<FileID, 1024U> Files;
+ HeaderIncludeCycleCheck &Check;
----------------
1024 entries is pushing the boat out for a small vector.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc/header-include-cycle.rst:63
+
+**Catch header cycles before they catch you - Try this Clang-tidy check
today!**
----------------
This tag line is just unnecessary
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144828/new/
https://reviews.llvm.org/D144828
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
