[PATCH] D47187: [clangd] Skip .inc headers when canonicalizing header #include.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

n



================
Comment at: clangd/index/CanonicalIncludes.cpp:50
+  if (I == Headers.end())
+    return Headers[0]; // Fallback to the defining header.
+  llvm::StringRef Header = *I;
----------------
nit: here and elsewhere, you probably want "declaring" rather than defining


================
Comment at: clangd/index/CanonicalIncludes.h:52
+  /// Returns the canonical include for symbol with \p QualifiedName. \p Headers
+  /// is a stack of #includes that starts from the last included header (i.e.
+  /// header that defines the symbol) to the header file that is directly
----------------
This might be clearer slightly more concretely:
```
\p Headers is the include stack: Headers.front() is the file declaring the symbol, and Headers.back() is the main file```
(or "included by the main file", but having it be main-file might be neater)


================
Comment at: clangd/index/CanonicalIncludes.h:54
+  /// header that defines the symbol) to the header file that is directly
+  /// included by the main file. When mapping headers, this skips files that are
+  /// not supposed to be #included directly e.g. .inc file.
----------------
Nit: "this" is a bit ambiguous here, the parameter or the algorithm?
Consider just dropping this new part of the comment, it's kind of an implementation detail (hiding behind the word "canonical")


================
Comment at: clangd/index/SymbolCollector.cpp:209
+  while (true) {
+    if (!Loc.isValid() || SM.isInMainFile(Loc))
+      break;
----------------
(as above, maybe want to include the main file for simplicity/symmetry)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47187