Szelethus marked 6 inline comments as done.
Szelethus 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:
----------------
NoQ wrote:
> 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.
I personally very much prefer the current state :/ It is locally defined on a
bottom of a file, I think I'll commit as is.
================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:965-966
+
+ It = CurrExpArgTokens.insert(
+ It, SuperExpArgTokens.begin(), SuperExpArgTokens.end());
+ std::advance(It, SuperExpArgTokens.size());
----------------
NoQ wrote:
> I think it's the first time in my life when i see a loop that (correctly)
> mutates the container :)
Yay ^-^
https://reviews.llvm.org/D52795
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
