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

Reply via email to