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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:134-135
+    do {
+      if (CurrentIt->Name.empty())
+        continue;
+
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:146
+      if (Location.isInvalid())
+        continue;
+
----------------
Again whats the rationale for using continue over break here?


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:148-150
+      Check.diag(Location, "'%0' included from command line",
+                 DiagnosticIDs::Note)
+          << CurrentIt->Name;
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp:154-161
+  static llvm::StringRef getFileName(llvm::StringRef FilePath) {
+    llvm::StringRef FileName = FilePath;
+    if (FileName.contains('/'))
+      FileName = FileName.rsplit('/').second;
+    if (FileName.contains('\\'))
+      FileName = FileName.rsplit('\\').second;
+    return FileName;
----------------
Dont reinvent the wheel - `llvm::sys::path::filename` does the same thing 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144828