[PATCH] D64915: [clangd] cleanup: unify the implemenation of checking a location is inside main file.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Neat! Many thanks, that's a very useful cleanup.
A few suggestions from my side.



================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:332
+bool isInsideMainFile(SourceLocation Loc, const SourceManager &SM) {
+  return Loc.isValid() && SM.isWrittenInMainFile(SM.getFileLoc(Loc));
+}
----------------
NIT: why not `getExpansionLoc`? Its implementation and description seems simpler and (I believe) it produces locations inside the same files as `getFileLoc` (as both the macro arg and the macro name of the expansion are always coming from the same file)


================
Comment at: clang-tools-extra/clangd/SourceCode.h:81
+///
+/// if the Loc is a macro location, the function calls getFileLoc to get the
+/// file location before checking.
----------------
Could we add a description without mentioning other `SourceManager` functions here?
I believe this should be a fair description of what this function is doing:
```
For macro locations, returns iff the macro is being expanded inside the main file.
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64915/new/

https://reviews.llvm.org/D64915