carlosgalvezp added a comment.
Amazing job @salman-javed-nz ! Here's some initial comments. Reviewing the
`NoLintPragmaHandler.cpp` will take some more time from me. It would have been
easier to see the diff if the contents had been moved as an NFC patch to a
separate file, and then applying the optimizations in this patch. But it's done
now so I'll deal with it :)
================
Comment at: clang-tools-extra/clang-tidy/CMakeLists.txt:20
GlobList.cpp
+ NoLintPragmaHandler.cpp
----------------
I think "Pragma" is a very specific term, used for example in `#pragma gcc
diagnostic` or `// IWYU pragma: keep`, but in `clang-tidy` we don't use the
word `pragma`, so that might be confusing. What about renaming it to
`NoLintHandler.cpp` or `NoLintDirectiveHandler.cpp`?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:125
+ const Diagnostic &Info,
+ SmallVectorImpl<NoLintError> &NoLintErrors);
+
----------------
Why not `SmallVector`? Sounds like the `Impl` is some "private" implementation?
================
Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.h:18
+namespace llvm {
+template <typename T> class SmallVectorImpl;
+} // namespace llvm
----------------
Why not `SmallVector`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116085/new/
https://reviews.llvm.org/D116085
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
