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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
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;
+    }
----------------
njames93 wrote:
> 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.
> 
> 
in theory it should raise an issue for self-include here..
and issue should be raised in Foo.hpp, can add check for this...


================
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 += "' -> ";
+    }
+
----------------
njames93 wrote:
> 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.
yes I wanted to do that, but at the point of FileChanged, where I fill up stack, i don't have an proper info where such file wre included.
It's possible, would need just to somehow connect info from InclusionDirective, assuming that when we got include, we go in such file, then probably keeping last processed include should be sufficient...


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:134
+private:
+  SmallVector<FileID, 1024U> Files;
+  HeaderIncludeCycleCheck &Check;
----------------
njames93 wrote:
> 1024 entries is pushing the boat out for a small vector.
Thats just 4KB of memory pre-allocated to avoid re-allocations. Small is "muten" here.


================
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!**
----------------
njames93 wrote:
> This tag line is just unnecessary
maybe it is unnecessary, but I just wanted to put some advertisement slogan for a check ...
Doesn't need to be bold... But it's is catchy...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144828/new/

https://reviews.llvm.org/D144828