[PATCH] D52742: [analyzer][WIP] Add macro expansions to the plist output

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

Szelethus marked 13 inline comments as done.
Szelethus added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:292
+
+  // Output the location.
+  FullSourceLoc L = P.getLocation().asLocation();
----------------
whisperity wrote:
> the location of what?
This is fairly obvious from the context, and is used throughout the whole file, so I'm leaving this one as is.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:737
+
+  for (auto It = MI->tokens_begin(), E = MI->tokens_end(); It != E;) {
+    Token T = *It;
----------------
whisperity wrote:
> Unnecessary `;` at the end?
It is actually necessary ^-^


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:765
+      if (MacroArgMap && MacroArgMap->count(II)) {
+        for (Token ExpandedArgT : MacroArgMap->at(II)) {
+          ExpansionOS << PP.getSpelling(ExpandedArgT) + ' ';
----------------
whisperity wrote:
> `Token&` so copies are explicitly not created?
I've seen `Token` being copied all over the place in other files, but after looking at it's implementation, const ref should be better.


================
Comment at: test/Analysis/plist-macros-with-expansion.cpp:24
+
+// CHECK: <string>Expanding macro 'SET_PTR_VAR_TO_NULL' to 'ptr = 0 '</string>
+
----------------
whisperity wrote:
> Why do we use the HTML entity tag `'` instead of `'`? Is the PList output expanded this much?
Yup, unfortunately.


https://reviews.llvm.org/D52742