Page MenuHomePhabricator

[analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded
ClosedPublic

Authored by Szelethus on Oct 2 2018, 12:14 PM.

Diff Detail

Event Timeline

Szelethus created this revision.Oct 2 2018, 12:14 PM
Szelethus updated this revision to Diff 168335.Oct 4 2018, 11:24 AM

Rebased to part 2.

I looked hard at this patch -- it is somewhat ridiculous that there's no builtin function for this -- but no matter how hard I try to look for something, I can't seem to find it.

I say this because I am aware that this patch isn't very nice to look at, but at least it works.

george.karpenkov requested changes to this revision.Oct 16 2018, 11:32 AM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
730

Please don't do this, inheritance from standard containers is a bad thing (tm)

This revision now requires changes to proceed.Oct 16 2018, 11:32 AM
Szelethus added inline comments.Oct 16 2018, 11:45 AM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
730

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?

Szelethus added inline comments.Oct 16 2018, 11:47 AM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
730

I mean, polymorphism, not inheritance.

NoQ accepted this revision.Nov 1 2018, 7:22 PM
NoQ added inline comments.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
730

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.

831

Not sure, would it make sense to take an rvalue reference instead of moving? Same for MacroName. Should be cheaper than moving.

934

Typo: Parentheses (i think.)

1023–1024

I think it's the first time in my life when i see a loop that (correctly) mutates the container :)

xazax.hun added inline comments.Nov 2 2018, 4:56 AM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
738

I wonder if the optional gives us any value here. An empty map could be just as great to represent that there are no arguments.

Szelethus set the repository for this revision to rC Clang.Nov 4 2018, 6:05 AM
Szelethus updated this revision to Diff 172559.Nov 5 2018, 3:15 AM

Addressing review comments!

Szelethus marked 6 inline comments as done.Nov 5 2018, 3:19 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
730

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.

1023–1024

Yay ^-^

Szelethus updated this revision to Diff 172561.Nov 5 2018, 3:20 AM
Szelethus marked 2 inline comments as done.
Szelethus marked 4 inline comments as done.

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.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
831

IS there any value of having Args here instead of Info.Args? I would just remove the Args reference here.

857

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.

982

Is the misspelling already committed? If not, better fix it in the other revision to keep this smaller. Otherwise, it is fine.

1018

Maybe a for loop mor natural here?

whisperity added inline comments.Nov 6 2018, 3:16 AM
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
1018

I asked this one already in the earlier (non-split) diff and the reason for the while is that there are a lot of conditional moves, advancements and even an erase call in the loop body.

Unfortunately, I found yet another corner case I didn't cover: if the macro arguments themselves are macros. I already fixed it, but it raises the question that what other things I may have missed? I genuinely dislike this project, and I fear that I can't test this enough to be be 100% it does it's job perfectly. Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is unnerving to put it lightly, but doing changes within Preprocessor is most definitely out of my reach.

Maybe it'd be worth to caution the users of this feature that it's experimental, but then again, it will always be, as long as there's no out-of-the-box solution from Preprocessor.

  • Added more documentation
  • Changed the nullpointer semantics as @xazax.hun pointed out
  • Realize that macro arguments themselves can be macros, handle them too.
  • Add a bunch more test. The above point actually fixed a wrong macro expansion is CALL_LAMBDA that somehow didn't get noticed. Gotta pay more attention when making the test files.
Szelethus marked 5 inline comments as done.Nov 14 2018, 11:17 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
982

Unfortunately the previous parts of this project are already commited :/

Szelethus marked 2 inline comments as done.Nov 14 2018, 11:26 AM

Ping, @xazax.hun, any objections?

xazax.hun accepted this revision.Nov 23 2018, 5:42 AM
xazax.hun added a reviewer: xazax.hun.

Some minor comment inline. Otherwise looks good.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
868

Maybe you could use find method to avoid repeated lookups? (you have 3 identical lookups in the map at the moment).

875

This II shadows the II in the outer scope. Maybe it would be better to give names correspond to the named notion, like ArgII to avoid confusion.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 26 2018, 6:31 PM
This revision was automatically updated to reflect the committed changes.
Szelethus marked 2 inline comments as done.Nov 26 2018, 6:31 PM