- User Since
- Aug 8 2019, 8:09 AM (68 w, 2 d)
Mon, Nov 2
Is anything planned or in progress that would address this issue?
Sep 29 2020
Reverted due to another obscure test failure. Working to diagnose.
I'm going to land this as-is, based on Bevin's LGTM and my own confidence that the 10 lines I added are correct (enough). @aaron.ballman , please reopen this review if you have additional comments or concerns. Thanks.
Sep 28 2020
Sep 23 2020
Sep 21 2020
Thanks for the quick feedback, Kristof! I changed the macro to be just a decrement operator, and triggered DBZ with that. Added the checks as you suggested.
Addressed feedback from @Szelethus
Sep 19 2020
Sep 18 2020
Sep 15 2020
Sep 14 2020
Sep 11 2020
Addressed all feedback from Aaron, except for two comments about reachability that I don't understand.
Addressed feedback from Aaron Ballman
Sep 10 2020
Thank you for the comments, @aaron.ballman . I'll update with the changes you requested shortly. I did have some requests for clarification of you, though. Thanks!
Sep 9 2020
Sep 4 2020
Thanks for the excellent feedback, @ebevhan . @aaron.ballman , @krememek , or @rsmith , could one of you take a look at this change and if it's acceptable, please approve it? I have not requested commit privileges yet, either, so I will need your assistance to land this change, assuming it's acceptable.
Sep 3 2020
NC push did not resolve failed test. Rebased in hopes that whatever has
broken the build has been resolved in the intervening commits.
NC. Pushing null change in hopes of re-triggering testing.
Refactored as ebevhan suggested to simplify patch.
Addressed reviewer feedback.
Sep 2 2020
Updating D86796: [Sema] Address-space sensitive index check for unbounded arrays
Sep 1 2020
I will tinker with the math to simplify as you suggest. Working with APInt and APSInt seems to promulgate sensitive and brittle code, which makes trying alternative expressions more tedious than I'd like (which is why I bailed on an earlier attempt to simplify this). However, that same observation about brittle code supports the goal that simpler math would be safer, as there would presumably be fewer opportunities for AP/APSInt to misbehave as they interact.
Aug 31 2020
Corrected formatting (per git-clang-format)
Aug 28 2020
Removed Change-Id from commit log message
Jun 2 2020
I reported this issue back in November, as https://bugs.llvm.org/show_bug.cgi?id=44143. This change fixes the bug -- I'll mark it as such.
Nov 26 2019
Nov 21 2019
Thank you, Erik. Appreciate the quick fix! :)
Nov 20 2019
Was this intended to affect the behavior of -Wswitch-bool? The following code did not trigger a warning with -Wswitch-bool prior to this commit, but now it does:
Nov 15 2019
Aug 25 2019
@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.