hokein added a comment. Left a few initial comments, it looks roughly good to me (for the macro-usage case, I might miss some historical context there, I think we come to an agreement that what this patch proposes is the designed behavior).
================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:75 + // - for a logical file like <vector>, we check Spelled + llvm::SmallVector<const Include *> match(Header H) const; + ---------------- sammccall wrote: > in the prototype I reimplemented this function in clangd, but I expect we can > just reuse the RecordedIncludes class instead, copying from clangd's includes > list is cheap. > > (That's one argument for putting this in a different header, which I can do > already if you prefer) that's an interesting idea, but we need to do it carefully because of `FileEntry*`, the `Include` structure has a filed of `FileEntry*`, it is not feasible for clangd to propagate it (clangd's `Inclusion` doesn't have it, instead it uses a `HeaderID` which is based on the `fs::UniqueID` to identify a physical file). But I think this is not something we should worry about at the moment, and the current interfaces look quite good. ================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:79 + std::vector<Include> All; + llvm::StringMap<llvm::SmallVector<unsigned>> BySpelling; + llvm::DenseMap<const FileEntry *, llvm::SmallVector<unsigned>> ByFile; ---------------- nit: worth a comment mentioning the unsigned is the index of `All`. ================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:118 + const FileEntry *Resolved = nullptr; // e.g. /path/to/c++/v1/vector + SourceLocation Location; // of hash in #include <vector> + unsigned Line = 0; // 1-based line number for #include ---------------- nit: HashLoc seems clearer than `Location`. And it looks like `Location` and `Line` feels somewhat redundant -- we can get the Line loc from the `Location` with `SourceManager`. But I think it is fair to hold both, Line is an important property of the include, which will probably widely used. ================ Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:159 + #define Y X + int one = x; + )cpp"); ---------------- why using a lower case `x` here? ================ Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:167 + + int one = ^X; + int uno = $exp^LATE; // a ref in LATE's expansion ---------------- this is a redef error with the one defined in the header, I think it is not intended, rename it? ================ Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:180 + Inputs.ExtraFiles["header.h"] = Header.code(); + Inputs.ErrorOK = true; // missing header + auto AST = build(); ---------------- IIUC, ErrorOK is irrelevant, and should be removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136723/new/ https://reviews.llvm.org/D136723 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
