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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
