[PATCH] D93222: [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis

[Gábor Horváth via Phabricator via cfe-commits]

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