ilya-biryukov added inline comments.

Comment at: clangd/Headers.cpp:45
+bool hasHeaderExtension(PathRef Path) {
+  constexpr static const char *HeaderExtensions[] = {".h", ".hpp", ".hh",
+                                                     ".hxx"};
There is another list of header extensions in 
`ClangdServer::switchSourceHeader`, let's reuse the list of extensions used 
here and there.

Comment at: clangd/Headers.cpp:48
+  return std::any_of(std::begin(HeaderExtensions), std::end(HeaderExtensions),
+                     [&Path](const char *Ext) { return Path.endswith(Ext); });
Maybe use `llvm::sys::path::extension` and search it in `HeaderExtensions` 
using `StringRef::equals_lower`?
Will also handle upper- and mixed-case.

Comment at: clangd/Headers.cpp:60
+                     IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+  if (File == Header || !hasHeaderExtension(Header))
+    return "";
Maybe replace `!hasHeaderExtension(Header)` to `hasSourceExtension(Header)`?
Knowing about all header extensions in advance is hard, knowing a subset of 
source extensions seems totally true.

Comment at: clangd/index/FileIndex.cpp:18
+CanonicalIncludes *CanonicalIncludesForSystemHeaders() {
+  auto *Includes = new CanonicalIncludes();
Maybe store a static inside the function body (to properly free memory) and 
return a const reference (to avoid mutating shared data)?

const CanonicalIncludes &CanonicalIncludesForSystemHeaders() {
  static CanonicalInclude Includes = []() -> CanonicalHeader {
    CanonicalInclude Includes;
    return Includes;
  return Includes;

Also Sam mentioned that `CanonicalIncludes` is not thread-safe, as the 
`Regex`'s `match()` mutates.

Comment at: unittests/clangd/FileIndexTests.cpp:173
+#ifndef LLVM_ON_WIN32
+TEST(FileIndexTest, CanonicalizeSystemHeader) {
Why not on Windows?

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to