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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 18 06:41:36 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list