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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice and precise work! And special thanks for the unit tests. I've found some minor nits.



================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:77
+  /// \param MacroExpansionLoc Must be the expansion location of a macro.
+  /// \return The textual representation of the token sequence which were
+  ///         substituted in place of the macro.
----------------
?


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:79-80
+  ///         substituted in place of the macro.
+  ///         If no macro was expanded at that location, returns an empty
+  ///         string.
+  MacroExpansionText
----------------
I think we should have an Optional<String> as a return type, so we'd be able to detect if no macro expansion happened. (?)


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:81
+  ///         string.
+  MacroExpansionText
+  getExpandedMacroForLocation(SourceLocation MacroExpansionLoc) const;
----------------
Could this be `StringRef` also?


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:87-88
+  ///         the macro expansion chain from the given location.
+  ///         If no macro was expanded at that location, returns an empty
+  ///         string.
+  StringRef
----------------
We should be able to distinguish between no macro expansion (1) and expansion to an empty string (2). So, I think we should have an Optional<StringRef> as a return type.


================
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.
----------------
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93222