hokein added inline comments.
================
Comment at: clangd/index/SymbolCollector.cpp:108
+std::string PrintableLoc(SourceLocation Loc, SourceManager &SM) {
+ if (Loc.isInvalid())
----------------
ioeric wrote:
> `PrintLoc`? `PrintableLoc` sounds like a checker for whether a location is
> printable.
`SourceLocation` has provided `printToString` API already, we don't need to
reinvent the wheel now.
================
Comment at: clangd/index/SymbolCollector.cpp:127
+ // location instead.
+ if (llvm::StringRef(PrintableLoc(Loc, SM)).startswith("<scratch")) {
+ FilePathStorage = makeAbsolutePath(
----------------
ioeric wrote:
> To reduce some code, consider
>
> ```
> Loc, Start, End = spelling locs;
> if (D->getLocation().isMacroID() && llvm::StringRef(PrintableLoc(Loc,
> SM)).startswith("<scratch")) {
> Loc, Start, End = expension locs;
> }
> // Generate SymbolLocation with Loc, Start, End
> ```
>
> Also, I think you could use `getLocStart` in place of `getLocation`?
There is no much duplicated code among the cases here (only one line of
generating the SymbolLocation). I prefer to the current way -- it shows the
logic a bit clearer, and also saves the cost of `getSpellingLoc` for the macro
case.
================
Comment at: unittests/clangd/Annotations.h:63
+ std::pair<std::size_t, std::size_t>
+ offsetRange(llvm::StringRef Name = "") const;
+
----------------
ioeric wrote:
> This doesn't seem to be necessary? I think you could use the `range` method
> above and convert it to offsets in the test matcher.
I think this is a useful method (probably be used in the future). Putting it to
test matcher, we have to pass `Code` parameter to the matcher
(`LocationOffset(range, Header.code())`) everywhere. Annotation seems to be the
best place.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42575
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
