- User Since
- Aug 8 2019, 8:09 AM (6 w, 1 d)
Sun, Aug 25
@Szelethus, firstly, thank you for landing this change. I really appreciate it! Secondly, apologies for misspelling your name earlier. And lastly, thank you for your patient explanation of how to get the diffs updated correctly in a Phabricator review. I think my mistake was that I made the change and the updates updates in a private branch, and not directly off master, and then duplicated the changes on master. For one of those sets of changes, I amended the first commit with the second round of changes, and I think I confused myself by doing that. In any case, lesson learned, and thank you again for your coaching!
Aug 21 2019
Kristoff, if you wouldn't mind, since you offered earlier, please go ahead and commit this change as-is, since it was accepted. I ran into some non-technical issues with my follow-up changes and I'm going to be unavailable for several weeks. To mitigate risk and work for my team, I'd like to submit the newer changes separately (and will reference this review in that changeset when I do, of course), after I return to work.
Aug 15 2019
Follow-up on reviewer feedback. Changed from blacklisting LValueToRValue to whitelisting IntegralCast. This was a good call -- additional testing with different cast kinds showed that the assertion tripped for other casts besides LValueToRValue, e.g., FloatToIntegral. I couldn't see any casts other than Integral where the enum check seemed appropriate. Testing with only IntegralCast enabled gave expected (correct) results.