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

Gábor Horváth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 6 01:30:07 PST 2018

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) {
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?


More information about the cfe-commits mailing list