[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Every review comment so far should be addressed now, with the exception of the following 2 points.



================
Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:420
+  // file if it is a <built-in>.
+  Optional<StringRef> FileName = SrcMgr.getNonBuiltinFilenameForID(File);
+  if (!FileName)
----------------
kadircet wrote:
> filenames in source manager could be misleading depending on how the file is accessed (there might be an override, symlinks, includemaps etc.) and different fileids can also refer to same file (e.g. when header is not include guarded). so both can result in reading the same file contents multiple times, but at least fileids are not strings so should be better keys for the cache && get rid of this step around fetching the filename from fileid.
> 
> Hence can we switch the cache from stringmap to a `densemap<fileid, vector<nolinttoken>>` instead.
> filenames in source manager could be misleading depending on how the file is accessed (there might be an override, symlinks, includemaps etc.) and different fileids can also refer to same file (e.g. when header is not include guarded). so both can result in reading the same file contents multiple times, but at least fileids are not strings so should be better keys for the cache && get rid of this step around fetching the filename from fileid.
> 
> Hence can we switch the cache from stringmap to a `densemap<fileid, vector<nolinttoken>>` instead.

I used FileIDs in an earlier attempt at this patch, but I had issues when specifying multiple input files to clang-tidy on the command line, e.g. `clang-tidy file1.cpp file2.cpp`. The analysis of each file begins with a fresh instance of SourceManager, so both file1.cpp and file2.cpp have a FileID of 1. It looks to me that I would need to clear the NoLintBlock cache each time a new SourceManager is used.

This is what the `nolintbeginend-multiple-TUs.cpp` test is all about.

The file path is a SourceManager-independent means of identifying the file for use in a map, so that's why it's being used. What are your thoughts on what map key to use? Neither looks ideal to me.



================
Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:469
+      (NoLint.type() == NoLintType::NoLintBegin)
+          ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+             "END' comment")
----------------
kadircet wrote:
> maybe just `'NOLINTBEGIN' comment without a matching 'NOLINTEND' comment`, vice versa for the other case.
> maybe just `'NOLINTBEGIN' comment without a matching 'NOLINTEND' comment`, vice versa for the other case.
Could we do this in another patch? Quite a number of unit tests will need updating.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085