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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
