[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 &apos;SET_PTR_VAR_TO_NULL&apos; to &apos;ptr 
= 0 &apos;</string>
+
----------------
whisperity wrote:
> Why do we use the HTML entity tag `&apos;` instead of `'`? Is the PList 
> output expanded this much?
Yup, unfortunately.


https://reviews.llvm.org/D52742



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits