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

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: gamesh411.
whisperity added a comment.

Your code looks good, just minor comments going on inline.
Trying to think of more cases to test for, in case someone generously misuses FLMs, as seen here in an example if you scroll down a bit: https://gcc.gnu.org/onlinedocs/cpp/Macro-Arguments.html#Macro-Arguments
Maybe one or two tests cases for this should be added, but I believe all corners are covered other than this.

(The whole thing, however, is generally disgusting. I'd've expected the `Preprocessor` to readily contain //a lot of stuff// that's going on here.)



================
Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:689
 
+  /// Returns with true if macros related to the bugpath should be expanded and
+  /// included in the plist output.
----------------
Returns ~~with ~~ true


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:261
+  std::string Expansion;
+  ExpansionInfo(std::string N, std::string E) : MacroName(N), Expansion(E) {}
+};
----------------
move move move like in your other self-defined type below


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:292
+
+  // Output the location.
+  FullSourceLoc L = P.getLocation().asLocation();
----------------
the location of what?


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:298
+
+  // Output the ranges (if any).
+  ArrayRef<SourceRange> Ranges = P.getRanges();
----------------
the ranges of what?


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:668
+/// need to expanded further when it is nested inside another macro.
+class MacroArgsInfo : public std::map<const IdentifierInfo *, ExpArgTokens> {
+public:
----------------
You have two records named "MacroArgsInfo" and "MacroNameAndArgsInfo", this is confusing. A different name should be for this class here... maybe the later used `MacroArgMap` is good, and then the struct's field should be renamed `ArgMap` or just `Args`.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:680
+                       llvm::Optional<MacroArgsInfo> M)
+    : Name(N), MI(MI), MacroArgMap(M) {}
+};
----------------
move string argument into local member (See https://www.bfilipek.com/2018/08/init-string-member.html, it's fascinating and me and @gamesh411  just found this again yesterday night.)


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:711-712
+static ExpansionInfo getExpandedMacroImpl(SourceLocation MacroLoc,
+                                      const Preprocessor &PP,
+                                      MacroArgsInfo *PrevArgMap) {
+
----------------
Indentation of these lines is wrong.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:723
+
+  // If this macro function-like.
+  if (MacroArgMap) {
----------------
"this is a" or "macro is function-like"


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:725
+  if (MacroArgMap) {
+    // If this macro is nested inside another one, let's manually expand it's
+    // arguments from the "super" macro.
----------------
its


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:726
+    // If this macro is nested inside another one, let's manually expand it's
+    // arguments from the "super" macro.
+    if (PrevArgMap)
----------------
What's a "\"super\" macro"? Maybe "outer" (or "outermost") is the word you wanted to use here?


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


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:756
+          while ((++It)->isNot(tok::r_paren)) {
+            assert(It->isNot(tok::eof));
+          }
----------------
`&& "Ill-formed input: macro args were never closed!"`


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:765
+      if (MacroArgMap && MacroArgMap->count(II)) {
+        for (Token ExpandedArgT : MacroArgMap->at(II)) {
+          ExpansionOS << PP.getSpelling(ExpandedArgT) + ' ';
----------------
`Token&` so copies are explicitly not created?


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:796
+  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(ExpanLoc);
+  const char *MacroNameBuf = LocInfo.second + BufferInfo.data();
+
----------------
Okay, this looks nasty. I get that pointer offsetting is commutative, but... this is nasty.

Also, what's the difference between using `.data()` and `.begin()`? `Lexer`'s this overload takes three `const char*` params. Maybe this extra variable here is useless and you could just pass `BufferInfo.data() + LocInfo.second` to the constructor below.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:796
+  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(ExpanLoc);
+  const char *MacroNameBuf = LocInfo.second + BufferInfo.data();
+
----------------
whisperity wrote:
> Okay, this looks nasty. I get that pointer offsetting is commutative, but... this is nasty.
> 
> Also, what's the difference between using `.data()` and `.begin()`? `Lexer`'s this overload takes three `const char*` params. Maybe this extra variable here is useless and you could just pass `BufferInfo.data() + LocInfo.second` to the constructor below.
Or we need a better name or a comment here.


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


Repository:
  rC Clang

https://reviews.llvm.org/D52742