ilya-biryukov added a comment.
Sorry about the delay. Thanks for the cleanup! I only have a few minor comments
here.
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:762
+ static const auto *SystemHeaderMap = [] {
+ const int Size = sizeof(IncludeMappings) / sizeof(IncludeMappings[0]);
+ auto *HeaderMap = new llvm::StringMap<llvm::StringRef>(Size);
----------------
Use `size_t`
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:764
+ auto *HeaderMap = new llvm::StringMap<llvm::StringRef>(Size);
+ for (int j = 0; j < Size; j++) {
+ auto Result = HeaderMap->insert(IncludeMappings[j]);
----------------
The name per LLVM should be `J` per LLVM style guide. Also, why not `I`?
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:768
+ // There should be no duplicates in IncludeMappings[].
+ assert(Result.second);
+ }
----------------
This will cause "unused" warning in builds with disabled assertions.
Add `(void)Result;` to avoid it.
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:768
+ // There should be no duplicates in IncludeMappings[].
+ assert(Result.second);
+ }
----------------
ilya-biryukov wrote:
> This will cause "unused" warning in builds with disabled assertions.
> Add `(void)Result;` to avoid it.
Use `assert(Result && "Duplicate in include mappings")`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125742/new/
https://reviews.llvm.org/D125742
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
