[PATCH] D125742: Minor refactor of CanonicalIncludes::addSystemHeadersMapping.

[Ilya Biryukov via Phabricator via cfe-commits]

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