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