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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
