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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
Herald added a subscriber: nullptr.cpp.

This is amazing. We longed for a sensible implementation for this for a long time. Really liking the unit tests as well!

There are a number of inlines you didn't mark as done but seem to have addressed.

> The main goal is to substitute the current macro expansion generator in the PlistsDiagnostics, but all the other DiagnosticsConsumer could benefit from this.

Good! Burn it, bury it, lets forget that it ever existed. It was a stupid idea from the get-go and got so much worse over time.



================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:64-66
+/// \remark Currently we don't respect the whitespaces between expanded tokens,
+///         so the output for this example might differ from the -E compiler
+///         invocation.
----------------
That might be very tricky. I recall stumbling across the same issue when using the `Preprocessor`. I think I used this or something similar:

https://clang.llvm.org/doxygen/classclang_1_1Preprocessor.html#adb53a8b33c6ea7a5e0953126da5fb121


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:91
+
+  /// \param MacroExpansionLoc Must be the expansion location of a macro.
+  /// \return The text from the original source code which were substituted by
----------------
Consider mentioning `getExpansionLoc`, since that is almost always the source of the expansion loc (no other comes to my mind).


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:41-43
+      if (Range.getBegin() == Range.getEnd())
+        return SM.getExpansionLoc(
+            MacroName.getLocation().getLocWithOffset(MacroName.getLength()));
----------------
Well that is interesting, so the `Range` parameter is **not** (always?) the range of the expansion. [[ https://clang.llvm.org/doxygen/classclang_1_1PPCallbacks.html#a1f99f55fc3c5df84b152f9bb2057633f | Doxygen]] says that 

> Called by Preprocessor::HandleMacroExpandedIdentifier when a macro invocation is found.

in which there is a TODO:

```
          // FIXME: We lose macro args info with delayed callback.
          Callbacks->MacroExpands(Info.Tok, Info.MD, Info.Range,
                                  /*Args=*/nullptr);
```

I suppose you're handling this corner case?


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:183-186
+    // FIXME: For now, we don't respect whitespaces between macro expanded
+    // tokens. We just emit a space after every identifier to produce a valid
+    // code for `int a ;` like expansions.
+    //              ^-^-- Space after the 'int' and 'a' identifiers.
----------------
In the  `TokenPrinter` in the plist implementation, I used `Preprocessor::getSpelling()`.


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

https://reviews.llvm.org/D93222