[PATCH] D144828: [clang-tidy] Add misc-header-include-cycle check

[Nathan James via Phabricator via cfe-commits]

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