This part focuses on expanding macro arguments. Unfortunately, I didn't find too many nice ways to do this, but hey, what can you expect from macros :(
Details
- Reviewers
george.karpenkov NoQ dkrupp rnkovacs baloghadamsoftware whisperity martong xazax.hun - Commits
- rG08d92e4a10ee: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded
rL347629: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded
rC347629: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded
Diff Detail
Event Timeline
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.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
680 | Please don't do this, inheritance from standard containers is a bad thing (tm) |
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
680 | 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 These are just a few. Do you insist? |
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
680 | I mean, polymorphism, not inheritance. |
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
680 | 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. | |
732 | Not sure, would it make sense to take an rvalue reference instead of moving? Same for MacroName. Should be cheaper than moving. | |
860 | Typo: Parentheses (i think.) | |
955–956 | I think it's the first time in my life when i see a loop that (correctly) mutates the container :) |
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
688 | 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. |
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 | ||
---|---|---|
732 | IS there any value of having Args here instead of Info.Args? I would just remove the Args reference here. | |
782–783 | 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. | |
931 | Is the misspelling already committed? If not, better fix it in the other revision to keep this smaller. Otherwise, it is fine. | |
938 | Maybe a for loop mor natural here? |
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
938 | 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.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
931 | Unfortunately the previous parts of this project are already commited :/ |
Some minor comment inline. Otherwise looks good.
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp | ||
---|---|---|
799 | Maybe you could use find method to avoid repeated lookups? (you have 3 identical lookups in the map at the moment). | |
806 | 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. |
Please don't do this, inheritance from standard containers is a bad thing (tm)