[PATCH] D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `(DRE.data() + any)`

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D143128#4167626 <https://reviews.llvm.org/D143128#4167626>, @jkorous wrote:

> This is an interesting topic. In the abstract I see the question as: "Should the Fix-Its prioritize how the code will fit the desired end state (presumably modern idiomatic C++) or carefully respect the state of the code as is now?"
>
> The only thing I feel pretty strongly about is that no matter what philosophy we decide to use here we should apply it consistently to all our Fix-Its (which might or might not already be the case).
>
> And FWIW I can also imagine at some point in the future we might either have two dialects of the Fix-Its or that a separate modernizer tool (completely independent of Safe Buffers) could suggest transformations like:
> "Would you like to change `&DRE.data()[any]` to `(DRE.data() + any)`?"



In D143128#4167655 <https://reviews.llvm.org/D143128#4167655>, @ymandel wrote:

> Fantastic topic! :)  In our experience at Google, we've generally followed the philosophy of leaving the code at least as good as it was before we touched it. So, if the idiom is archaic, but we're just adjusting it, that's fine (as in this example), but our tools shouldn't generate non-idiomatic (or anti-idiomic) code.  We've also often taken the path of "leave cleanups to a separate pass", especially when we already have say, a clang tidy check, that does that kind of clean up running regularly.  But, this one is more a judgment call. I'd probably lean towards "make the code better once you're at it", but certainly see the conservative argument.

That's a fantastic topic in general, but in *this* case I'm really not sure which one's ultimately "better", I'd totally write code both ways and at different times prefer one over the other.

I honestly actually prefer code like `&DRE[any]`/`&DRE.data()[any]` to the one with pointer arithmetic, specifically because it avoids the subject of pointer arithmetic, but instead talks about "Let's take this object, for which we already have a name, and take its address". The readers don't have to typecheck the expression in their heads in order to figure out whether correct offset multipliers are applied during pointer arithmetic. Another advantage of `&DRE.data()[any]` is that it's really easy to transform it to idiomatic `&DRE[any]` once the user confirms that the bounds checks don't actually get in the way in their case. This is much harder to achieve with `DRE.data() + any` given that `DRE + any` isn't valid code anymore.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143128/new/

https://reviews.llvm.org/D143128