- User Since
- Mar 5 2019, 4:12 AM (173 w, 3 d)
Mon, Jun 20
Use IsVariadic when emitting a location for an entry value.
Fri, Jun 17
Mon, Jun 13
Wed, Jun 8
Update name and add docstring to requested function.
Mon, Jun 6
Jun 1 2022
Make some changes to correctly handle the float_range=0.0 case, add tests to cover it, and change comment as requested.
May 23 2022
Remove unused "zero range" test.
May 19 2022
May 13 2022
Apr 28 2022
Apr 27 2022
Apr 26 2022
Sorry for the delay on approving this, LGTM!
Apr 25 2022
Good fix, LGTM.
Apr 20 2022
Still a further suggestion on the most recent change - it's a minor point, but necessary I think to not produce invalid expressions. Other than that, LGTM.
Apr 11 2022
Apr 6 2022
Mar 31 2022
Good patch - I've left quite a few comments, but most of them are minor nits and some are not requirements for this patch to be merged.
Mar 21 2022
Thanks for the change, LGTM
Latest change makes sense, LGTM
Mar 7 2022
Mar 1 2022
Feb 28 2022
Feb 23 2022
Feb 9 2022
LGTM apart from the comment below.
Simple fix LGTM.
Feb 2 2022
Seems reasonable as a fix to me. Is there any particular reason that the undef value test couldn't exist within the original function, or are they separated to look cleaner? YMMV and I don't feel strongly either way, so LGTM.
Jan 31 2022
LGTM, that's a depressingly good performance gain/prevented cost.
Seems like a good change - the compile time improvement is nice, and this doesn't look like it should have a noticeable memory footprint. LGTM with the other comments addressed.
Jan 28 2022
LGTM, nice performance improvement!
Jan 26 2022
LGTM, with a couple of nits - good work!
Jan 25 2022
Just noting here (for future reference) the above issue should be fixed as of ea17d29a6c83.
Jan 24 2022
Jan 17 2022
Jan 14 2022
Performance seems like a serious issue when working with large projects here, but I have some questions/thoughts about this approach:
Jan 10 2022
Questions around testing aside, the code changes here LGTM.
Dec 15 2021
Change SGTM, except for the minor inline comments.
Trivial fix, LGTM.
Apologies, didn't see the existing comments on this review before submitting - given that the concern has been raised by another, it may be worth making a change to confirm that debug values have been dropped, and also check the error message. A simple way to do this might be to run the test twice, using -debugify-level=locations-and-variables for the second run, with a directive CHECK: drops dbg.value()/dbg.declare() - this should solve both issues by ensuring the test only passes if we are actually dropping debug info, and we are checking for the correct error message.
Aside from the mentioned nit, LGTM.
Seems like a simple and sensible change.
Dec 14 2021
Add a lit test for llvm-link, move the unit test changes into a separate function (leaving the original unmodified). The long comment in the original test has been duplicated (with a minor change) in the new test - I figure it's better to have each test containing the relevant info directly, but if preferred I could change it to some kind of "see above" instead.
Dec 13 2021
Dec 6 2021
Dec 3 2021
It's a relatively obscure instruction I think, but we should also handle callbr in the same manner as invoke, as that is also a terminator instruction that defines an SSA value. A similar solution should suffice, as it also has a single destination where the value is expected to be valid (getDefaultDest()) that the dbg.declare can be inserted at the start of.
Dec 2 2021
Dec 1 2021
Nov 25 2021
Would it make sense to have these tests run twice, once with experimental-debug-variable-locations being true and once with false? Otherwise it looks like they will no longer cover the normal DBG_VALUE case with this patch.
Looks like a plain and simple performance improvement, LGTM (one minor nit included).
Nov 24 2021
Nov 22 2021
Nov 16 2021
Nov 12 2021
Add 1 more test for the hit_count argument.
Nov 11 2021
Move the generation of the address resolution map to Heuristic (None initialized on command objects until the heuristic runs). Added address information to the verbose output (only prints additional info if DexDeclareAddress is actually present), and always print the name of the address in the "missing values" and "encountered expected values" output (implementation for misordered values is more complex, and has been ignored). Also, removed some unused functions in DexExpectWatchBase.
Found one issue with the test (noted inline), but otherwise no issues.
Seems straightforward enough to me, the code block in question clearly only cares about preg defs for the purposes of sinking. LGTM, with one inline nit.
Add a set of feature tests for DexDeclareAddress; also fix a minor error that would appear if the address was never resolved.
Nov 10 2021
Nov 9 2021
LGTM! Good catch on this one, I thought I'd removed all the DenseMap iteration errors but I guess this one slipped by. There's a small suggestion in the inline comments, but feel free to ignore it - it's more of a preference than a rule. Shouldn't need a test either since this patch is trivial and solves a determinism issue, so this should be fine as-is.