Refactor caching walker to make creating a walker that skips the
starting access strightforward.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 26365 Build 26364: arc lint + arc unit
Event Timeline
Thanks for this!
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
1000 | 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. | |
1001 | please clang-format this | |
1003 | nit: no trailing semi, please | |
1479 | No trailing ;, please | |
2293 | 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 | ||
---|---|---|
961 | 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.? | |
965 | Since this is already a nested class, is this necessary? I don't use friend often enough to know for sure | |
2231 | nit: no else after return, please | |
2285 | Please also note whether SkipSelf was true/false in these debug printouts | |
2292 | 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) | |
2298 | please clang-format (I think it prefers empty lines between functions) |
lib/Analysis/MemorySSA.cpp | ||
---|---|---|
2298 | 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.?