salman-javed-nz added a comment. In D116085#3257210 <https://reviews.llvm.org/D116085#3257210>, @carlosgalvezp wrote:
> 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 :) Thank you :) You might find the previous revision of this patch useful then (https://reviews.llvm.org/D116085?id=395606). In this earlier revision I made the change in the original file `ClangTidyDiagnosticHandler.cpp`. There's been some refinements to that code in the subsequent revisions (the subsequent revisions aren't merely just moving the changes to a separate file) but it's still useful as a way to get the general gist of the new NOLINT tokenization and location caching process. ================ Comment at: clang-tools-extra/clang-tidy/CMakeLists.txt:20 GlobList.cpp + NoLintPragmaHandler.cpp ---------------- carlosgalvezp wrote: > 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`? Your suggestions sound better. I'm not attached to the name `NoLintPragmaHandler`. ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:125 + const Diagnostic &Info, + SmallVectorImpl<NoLintError> &NoLintErrors); + ---------------- carlosgalvezp wrote: > Why not `SmallVector`? Sounds like the `Impl` is some "private" > implementation? `SmallVectorImpl` is the recommended type to use for output params. - `SmallVector<T>` is actually `SmallVectorImpl<T, N = some_default_size>` under the hood. - By declaring the function param as `SmallVectorImpl` I'm saying that I don't care about the size of the vector that the caller passes in. See https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h ``` Even though it has “Impl” in the name, SmallVectorImpl is widely used and is no longer “private to the implementation”. ``` ================ Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.h:18 +namespace llvm { +template <typename T> class SmallVectorImpl; +} // namespace llvm ---------------- carlosgalvezp wrote: > Why not `SmallVector`? See my other comment about `SmallVectorImpl`. 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