[PATCH] D108893: clang-tidy: introduce readability-containter-data-pointer check

Mon Aug 30 11:14:54 PDT 2021

aaron.ballman added a comment.

In D108893#2972651 <https://reviews.llvm.org/D108893#2972651>, @Eugene.Zelenko wrote:

> In D108893#2972634 <https://reviews.llvm.org/D108893#2972634>, @compnerd wrote:
>
>> @Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and assuming that he's okay with `modernize.container-data-pointer`, I'll change it to that.
>
> Sure, Aaron has much broader vision than me.

Lies! I just wear glasses, that's all. ;-)

I can see a case being made for either `modernize` or `readability`. For `modernize`, `data()` was newly added in C++11, so you could argue that this is used to modernize C++98 code. For `readability`, `data()` makes the semantics of the underlying code more clear. Also, `bugprone` makes sense too because doing `&v[0]` is UB if `v` is empty, but `v.data()` is not (immediately) UB.

I think `bugprone` would be a tough place to put it because of false positive concerns. This is more of a blanket code rewriting utility than it is trying to catch bugs (the static analyzer and UBSan are better tools for that in that case).
I think `modernize` would be reasonable, but given that C++11 is already over ten years old, I feel like this is losing its "modernization" aspects in some ways. Also, this only covers converting `&vec[0]` into a call to `data()`; for `modernize`, I'd expect it to convert `&vec[N]` into `vec.data() + N` more broadly.
I think `readability` makes slightly more sense because then we can position this as a rewriting tool to make the code more readable (so there are no false positives -- it transforms all `&vec[0]` calls to `data()`).

However, if someone wants to make a strong case for `modernize`, I think it'd be defensible. Also, if someone wanted to suggest we add it to both `readability` and `modernize`, that could also make sense (perhaps with an option to convert `&vec[N]` that's on by default for modernize and off by default for readability).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108893