[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded
Artem Dergachev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 1 19:22:22 PDT 2018
NoQ accepted this revision.
NoQ added inline comments.
================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:677
+/// need to expanded further when it is nested inside another macro.
+class MacroArgMap : public std::map<const IdentifierInfo *, ExpArgTokens> {
+public:
----------------
Szelethus wrote:
> Szelethus wrote:
> > george.karpenkov wrote:
> > > Please don't do this, inheritance from standard containers is a bad thing (tm)
> > Actually, I disagree with you on this one. For ease of use, these constructs are used all over the place in the LLVM codebase, and since I never do anything inheritance related, this shouldn't hurt much.
> >
> > https://clang.llvm.org/doxygen/classclang_1_1ento_1_1PathPieces.html
> > http://llvm.org/doxygen/classllvm_1_1PredicateBitsetImpl.html
> > http://llvm.org/doxygen/classllvm_1_1HexagonBlockRanges_1_1RangeList.html
> >
> > These are just a few.
> >
> > Do you insist?
> I mean, polymorphism, not inheritance.
Dunno why exactly, but George really hates these :)
To me it's a reasonable thing to use in a tiny utility class - as opposed to re-implementing all vector methods in a composition every time you need just one extra method.
It should also be possible to avoid this by sacrificing object-oriented-ness by turning the newly added method into a standalone function, i.e.:
```
using MacroArgMap = std::map<const IdentifierInfo *, ExpArgTokens>;
void expandFromPrevMacro(MacroArgMap &This, const MacroArgMap &Super);
```
Which also seems almost free.
================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:772
const MacroInfo *MI = Info.MI;
+ llvm::Optional<MacroArgMap> Args = std::move(Info.Args);
+
----------------
Not sure, would it make sense to take an rvalue reference instead of moving? Same for `MacroName`. Should be cheaper than moving.
================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:876
+ // parantheses, so we'll count how many parantheses aren't closed yet.
+ int ParanthesesDepth = 1;
+
----------------
Typo: Parentheses (i think.)
================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:965-966
+
+ It = CurrExpArgTokens.insert(
+ It, SuperExpArgTokens.begin(), SuperExpArgTokens.end());
+ std::advance(It, SuperExpArgTokens.size());
----------------
I think it's the first time in my life when i see a loop that (correctly) mutates the container :)
https://reviews.llvm.org/D52795
More information about the cfe-commits
mailing list