[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) {
++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
More information about the cfe-commits
mailing list