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