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