[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I would love to see a test with deeper macro in macro expansion and larger number of arguments, with some of the arguments unused. Some minor nits inline, otherwise looks good.



================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:831
   const MacroInfo *MI = Info.MI;
+  MacroArgMap &Args = Info.Args;
+
----------------
IS there any value of having `Args` here instead of `Info.Args`? I would just remove the `Args reference here.`


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:857
                          getMacroInfoForLocation(PP, SM, II, T.getLocation())) {
-      getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP);
+      getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, PrevArgs);
 
----------------
Maybe instead of mutating PrevArgs above, we could keep `PrevArgs` argument to point to the previous arguments and pass the address of `Info.Args` here? Do we need the null pointer semantics? Expanding macros from an empty map should be noop. Maybe we could eliminate some branches this way.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:982
+  int ParenthesesDepth = 1;
+  while (ParenthesesDepth != 0) {
     ++It;
----------------
Is the misspelling already committed? If not, better fix it in the other revision to keep this smaller. Otherwise, it is fine.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1018
+    auto It = CurrExpArgTokens.begin();
+    while (It != CurrExpArgTokens.end()) {
+      if (It->isNot(tok::identifier)) {
----------------
Maybe a for loop mor natural here?


https://reviews.llvm.org/D52795