[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D45702#1086802, @shuaiwang wrote:

> In https://reviews.llvm.org/D45702#1086250, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D45702#1085890, @shuaiwang wrote:
> >
> > > In https://reviews.llvm.org/D45702#1085224, @aaron.ballman wrote:
> > >
> > > > > Have you run this over any large code bases to see whether the check triggers in practice?
> > > >
> > > > I'm still curious about this, btw.
> > >
> > >
> > > Yes it triggers in Google's code base.
> >
> >
> > Were there any false positives that you saw?
>
>
> From randomly checking several triggerings no I didn't find any false positives. I feel the check should be pretty safe in terms of false positives because we only trigger on configured types.


Good to hear, thank you for the information.



================
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+                  anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)))))),
+              callee(namedDecl(hasName("data"))))
+              .bind("call")))),
----------------
shuaiwang wrote:
> aaron.ballman wrote:
> > Eugene.Zelenko wrote:
> > > aaron.ballman wrote:
> > > > Should this check apply equally to `std::string::c_str()` as well as `std::string::data()`?
> > > readability-redundant-string-cstr do both.
> > Yup! But that makes me wonder if the name "redundant-data-call" is an issue. Perhaps the check name should focus more on the array subscript in the presence of an operator[]()?
> How about "readability-circumlocutionary-subscript"?
> "readability-circumlocutionary-element-access"?
> "circumlocutionary" -> "verbose"?
hah, I think circumlocutionary might be a bit too much. ;-) I think `readability-simplify-array-subscript-expr` might be reasonable, however. Right now, the simplification is just for `foo.data()[0]` but it seems plausible that there are other array subscript simplifications that could be added in the future, like `a[1 + 1]` being converted to `a[2]` or `x ? a[200] : a[400]` going to `a[x ? 200 : 400]` (etc).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45702