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;
addSystemHeadersMapping(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?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43462
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
