Refactor caching walker to make creating a walker that skips the
starting access strightforward.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 26193 Build 26192: arc lint + arc unit
Event Timeline
Thanks for this!
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
976 | ClobberWalker is pretty big (nearly 2K). Can we instead make SkipSelfWalker carry around a pointer to MSSA->getWalker() (or just MSSA. I'm not picky)? Since CachingWalker is opaque (modulo overrides from MemorySSAWalker) to everything that isn't in this file, I have no issue with adding "don't touch these unless ..." methods to its public API. | |
977 | please clang-format this | |
979 | nit: no trailing semi, please | |
1462 | No trailing ;, please | |
2275 | Please do the refactoring/dedup as a part of this patch, rather than as a follow up |
This LGTM at a high level; just a handful of nits for you, and I think this should be good to submit
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
959 | nit: if we don't have commentary about SkipSelf elsewhere, can we please have a description of why it exists/what it's intended to do/etc.? | |
963 | Since this is already a nested class, is this necessary? I don't use friend often enough to know for sure | |
2233 | nit: no else after return, please | |
2274 | nit: if member functions are one or two lines like this, please define them in the class itself (unless there's some code ordering-related reason not to) | |
2275 | Please also note whether SkipSelf was true/false in these debug printouts | |
2280 | please clang-format (I think it prefers empty lines between functions) |
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
2280 | clang-format doesn't care about this, but I added a line, seems better. |
nit: if we don't have commentary about SkipSelf elsewhere, can we please have a description of why it exists/what it's intended to do/etc.?