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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked 4 inline comments as done.
PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:134-135
+    do {
+      if (CurrentIt->Name.empty())
+        continue;
+
----------------
njames93 wrote:
> Is this a case that can ever come up?
> If it does come up, surely any notes emitted after that would be garbage, in which case wouldn't it make more sense to break rather than continue?
I will remove this...


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:146
+      if (Location.isInvalid())
+        continue;
+
----------------
njames93 wrote:
> Again whats the rationale for using continue over break here?
Idea is to provide any information, even if it could be missing something.


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:148-150
+      Check.diag(Location, "'%0' included from command line",
+                 DiagnosticIDs::Note)
+          << CurrentIt->Name;
----------------
njames93 wrote:
> Is this a case that is expected to come up, if so can there be a test added.
> Likewise if we do have a file that was included from the command line,  surely we should break after emitting that diagnostic, It's not like the command line can be included from another file.
I didn't found any case it could be, added this as an preclusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144828