xazax.hun added a comment.
Overall looks good to me, I have some minor nits and questions inline.
================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:22
+ : PP(PP), SM(PP.getSourceManager()), LangOpts(LangOpts) {
+ class MacroExpansionRangeRecorder : public PPCallbacks {
+ const Preprocessor &PP;
----------------
It may be more idiomatic to put classes in an anonymous namespace rather than
expanding them in a method.
================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:33
+
+ void MacroExpands(const Token &MacroName, const MacroDefinition &MD,
+ SourceRange Range, const MacroArgs *Args) override {
----------------
Will we end up recording ranges for intermediate macro expansions? If yes, we
might end up recording excessive amount of ranges. Is there any way to only
record final expansion ranges in that case?
================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:39
+
+ SourceLocation ExpansionEnd = [Range, &SM = SM, &MacroName] {
+ // If the range is empty, use the length of the macro.
----------------
martong wrote:
> ?
Why do you need the assignment in `&SM = SM`?
================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:94
+ if (it == ExpandedTokens.end())
+ return MacroExpansionText("");
+ return it->getSecond();
----------------
I am a bit uneasy about this. As far as I understand a macro could be expanded
to an empty string. How could the user differentiate between a valid empty
expansion and a lookup failure. Also, can this lookup ever fail in a well
behaving code? Do we need to add `llvm_unreachable()` or something?
================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:105
+ if (it == ExpansionRanges.end())
+ return "";
+
----------------
Similar concerns to above. Do we want an assert? When can this happen?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93222/new/
https://reviews.llvm.org/D93222
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
