[PATCH] D91025: [clangd] Fix locateMacroAt() for macro definition outside preamble

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 8 06:45:01 PST 2020


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

Nice catch! As always, sorry about the delay.



================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:987
+  FileID FID = SM.getFileID(Loc);
+  auto JustAfterToken =
+      SM.getLocForEndOfFile(FID) == Loc ? Loc : Loc.getLocWithOffset(1);
----------------
nit: I guess these would be clearer with auto -> SourceLocation


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:988
+  auto JustAfterToken =
+      SM.getLocForEndOfFile(FID) == Loc ? Loc : Loc.getLocWithOffset(1);
+  auto *MI =
----------------
I'm a little confused about the condition here: Loc points at the macro name, which must be at least 1 character long, so we can't be at EOF, right?

Unless i'm missing something, I think we can promote to an assert


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:993
+    auto JustBeforeToken =
+        SM.getLocForStartOfFile(FID) == Loc ? Loc : Loc.getLocWithOffset(-1);
+    MI = PP.getMacroDefinitionAtLoc(IdentifierInfo, JustBeforeToken)
----------------
on the other hand, I suppose this case *is* possible with e.g. a builtin macro used at the start of the file.

I think hoisting this into the condition might be clearer by avoiding the redundant case (the actual *performance* doesn't matter, just for readability)
```
// If this is foo in `#undef foo`, use the old definition.
if (!MI && SM.getLocForStartOfFile(FID) != Loc)
  MI = PP.getMacroDefinitionAtLoc(II, Loc.getLocWithOffset(-1)).getMacroInfo();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91025



More information about the cfe-commits mailing list