[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+ anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
+ callee(namedDecl(hasName("data"
+ .bind("call",
shuaiwang wrote:
> aaron.ballman wrote:
> > 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).
> Just `readability-simplify-subscript-expr`?
> Since after simplification the subscript operation is done by calling
> overloaded `operator[]` on an object instead of built-in subscript operator
> on an array.
>
> Let me know if this name looks good to you and I'll do the actual renaming
> (together with addressing other comments) after your confirmation.
I think `readability-simplify-subscript-expr` is a reasonable name.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang added inline comments.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+ anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
+ callee(namedDecl(hasName("data"
+ .bind("call",
aaron.ballman wrote:
> 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).
Just `readability-simplify-subscript-expr`?
Since after simplification the subscript operation is done by calling
overloaded `operator[]` on an object instead of built-in subscript operator on
an array.
Let me know if this name looks good to you and I'll do the actual renaming
(together with addressing other comments) after your confirmation.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang added a comment. 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
aaron.ballman added a comment. 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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang added a comment. 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang added inline comments.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+ anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
+ callee(namedDecl(hasName("data"
+ .bind("call",
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"?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:31
+void RedundantDataCallCheck::registerMatchers(MatchFinder *Finder) {
+ using namespace ast_matchers;
+
No need to register any of these matchers unless in C++ mode.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+ anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
+ callee(namedDecl(hasName("data"
+ .bind("call",
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[]()?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
Eugene.Zelenko added inline comments.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+ anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
+ callee(namedDecl(hasName("data"
+ .bind("call",
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
aaron.ballman added a comment.
> Have you run this over any large code bases to see whether the check triggers
> in practice?
I'm still curious about this, btw.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:45
+ anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
+ callee(namedDecl(hasName("data"
+ .bind("call",
Should this check apply equally to `std::string::c_str()` as well as
`std::string::data()`?
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:57
+ const auto *Member = Result.Nodes.getNodeAs("member");
+ auto DiagBuilder = diag(Member->getMemberLoc(), "redundant call to .data()");
+ if (Member->isArrow()) {
I think the word "redundant" isn't going to be immediately obvious to a user
who would write code like that. Perhaps `"accessing an element of the container
does not require a call to 'data()'; did you mean to use 'operator[]()'
instead?"`
Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:7
+This check finds and suggests removing redundant ``.data()`` calls. Currently
+this covers calling ``.data()`` and immediately doing array subscript operation
+to obtain a single element, in which case simply calling ``operator[]``
suffice.
doing array subscript -> doing an array subscript
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
Eugene.Zelenko added inline comments. Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:22 + +const char kDefaultTypes[] = +"::std::basic_string;::std::basic_string_view;::std::vector;::std::array"; shuaiwang wrote: > Eugene.Zelenko wrote: > > Actually you should use static, not anonymous namespace for variables and > > functions. > Done, but why? See [[ https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | LLVM coding standards ]]. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang added inline comments. Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:22 + +const char kDefaultTypes[] = +"::std::basic_string;::std::basic_string_view;::std::vector;::std::array"; Eugene.Zelenko wrote: > Actually you should use static, not anonymous namespace for variables and > functions. Done, but why? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang updated this revision to Diff 143207.
shuaiwang marked 7 inline comments as done.
shuaiwang added a comment.
Addressed more comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantDataCallCheck.cpp
clang-tidy/readability/RedundantDataCallCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-redundant-data-call.rst
test/clang-tidy/readability-redundant-data-call.cpp
Index: test/clang-tidy/readability-redundant-data-call.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-data-call.cpp
@@ -0,0 +1,108 @@
+// RUN: %check_clang_tidy %s readability-redundant-data-call %t \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: readability-redundant-data-call.Types, \
+// RUN: value: '::std::basic_string;::std::basic_string_view;MyVector'}]}" --
+
+namespace std {
+
+template
+class basic_string {
+ public:
+ using size_type = unsigned;
+ using value_type = T;
+ using reference = value_type&;
+ using const_reference = const value_type&;
+
+ reference operator[](size_type);
+ const_reference operator[](size_type) const;
+ T* data();
+ const T* data() const;
+};
+
+using string = basic_string;
+
+template
+class basic_string_view {
+ public:
+ using size_type = unsigned;
+ using const_reference = const T&;
+ using const_pointer = const T*;
+
+ constexpr const_reference operator[](size_type) const;
+ constexpr const_pointer data() const noexcept;
+};
+
+using string_view = basic_string_view;
+
+}
+
+template
+class MyVector {
+ public:
+ using size_type = unsigned;
+ using const_reference = const T&;
+ using const_pointer = const T*;
+
+ const_reference operator[](size_type) const;
+ const T* data() const noexcept;
+};
+
+#define DO(x) do { x; } while (false)
+#define ACCESS(x) (x)
+#define GET(x, i) (x).data()[i]
+
+template
+class Foo {
+ public:
+ char bar(int i) {
+return x.data()[i];
+ }
+ private:
+ T x;
+};
+
+void f(int i) {
+ MyVector v;
+ int x = v.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call to .data() [readability-redundant-data-call]
+ // CHECK-FIXES: int x = v[i];
+
+ std::string s;
+ char c1 = s.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant call to .data()
+ // CHECK-FIXES: char c1 = s[i];
+
+ std::string_view sv;
+ char c2 = sv.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call to .data()
+ // CHECK-FIXES: char c2 = sv[i];
+
+ std::string* ps =
+ char c3 = ps->data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant call to .data()
+ // CHECK-FIXES: char c3 = (*ps)[i];
+
+ char c4 = (*ps).data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant call to .data()
+ // CHECK-FIXES: char c4 = (*ps)[i];
+
+ DO(char c5 = s.data()[i]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: redundant call to .data()
+ // CHECK-FIXES: DO(char c5 = s[i]);
+
+ char c6 = ACCESS(s).data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: redundant call to .data()
+ // CHECK-FIXES: char c6 = ACCESS(s)[i];
+
+ char c7 = ACCESS(s.data())[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+ // CHECK-FIXES: char c7 = ACCESS(s)[i];
+
+ char c8 = ACCESS(s.data()[i]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+ // CHECK-FIXES: char c8 = ACCESS(s[i]);
+
+ char c9 = GET(s, i);
+
+ char c10 = Foo{}.bar(i);
+}
Index: docs/clang-tidy/checks/readability-redundant-data-call.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-data-call.rst
@@ -0,0 +1,23 @@
+.. title:: clang-tidy - readability-redundant-data-call
+
+readability-redundant-data-call
+===
+
+This check finds and suggests removing redundant ``.data()`` calls. Currently
+this covers calling ``.data()`` and immediately doing array subscript operation
+to obtain a single element, in which case simply calling ``operator[]`` suffice.
+
+Examples:
+
+.. code-block:: c++
+
+ std::string s = ...;
+ char c = s.data()[i]; // char c = s[i];
+
+Options
+---
+
+.. option:: Types
+
+ The list of type(s) that triggers this check. Default is
+ `::std::basic_string;::std::basic_string_view;::std::vector;::std::array`
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -218,6 +218,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+ readability-redundant-data-call
readability-redundant-declaration
readability-redundant-function-ptr-dereference
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
Eugene.Zelenko added inline comments. Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:22 + +const char kDefaultTypes[] = +"::std::basic_string;::std::basic_string_view;::std::vector;::std::array"; Actually you should use static, not anonymous namespace for variables and functions. Comment at: docs/ReleaseNotes.rst:138 +- New :doc:`readability-redundant-data-call + ` check r is after p :-) Comment at: docs/ReleaseNotes.rst:141 + + This check finds and suggests removing redundant `.data()` calls. + Please remove //This check// and enclose .data() in ``, not `. Same for documentation. Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:6 + +This check finds and suggests removing redundant `.data()` calls. +Currently this covers calling `.data()` and immediately doing array subscript Please use as much of 80 characters as possible. Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:8 +Currently this covers calling `.data()` and immediately doing array subscript +operation to obtain a single element, in which case simply calling `operator[]` +suffice. operator[] should be enclosed in ``, not `. Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:14 +.. code-block:: c++ + std::string s = ...; + char c = s.data()[i]; // char c = s[i]; Please insert empty line above. Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:22 + + The list of type(s) that triggers this check. Default covers `std::string`, + `std::string_view`, `std::vector`, `std::array`. I think will be good idea to specify default value as complete string. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang updated this revision to Diff 143204.
shuaiwang marked 7 inline comments as done.
shuaiwang added a comment.
Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantDataCallCheck.cpp
clang-tidy/readability/RedundantDataCallCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-redundant-data-call.rst
test/clang-tidy/readability-redundant-data-call.cpp
Index: test/clang-tidy/readability-redundant-data-call.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-data-call.cpp
@@ -0,0 +1,108 @@
+// RUN: %check_clang_tidy %s readability-redundant-data-call %t \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: readability-redundant-data-call.Types, \
+// RUN: value: '::std::basic_string;::std::basic_string_view;MyVector'}]}" --
+
+namespace std {
+
+template
+class basic_string {
+ public:
+ using size_type = unsigned;
+ using value_type = T;
+ using reference = value_type&;
+ using const_reference = const value_type&;
+
+ reference operator[](size_type);
+ const_reference operator[](size_type) const;
+ T* data();
+ const T* data() const;
+};
+
+using string = basic_string;
+
+template
+class basic_string_view {
+ public:
+ using size_type = unsigned;
+ using const_reference = const T&;
+ using const_pointer = const T*;
+
+ constexpr const_reference operator[](size_type) const;
+ constexpr const_pointer data() const noexcept;
+};
+
+using string_view = basic_string_view;
+
+}
+
+template
+class MyVector {
+ public:
+ using size_type = unsigned;
+ using const_reference = const T&;
+ using const_pointer = const T*;
+
+ const_reference operator[](size_type) const;
+ const T* data() const noexcept;
+};
+
+#define DO(x) do { x; } while (false)
+#define ACCESS(x) (x)
+#define GET(x, i) (x).data()[i]
+
+template
+class Foo {
+ public:
+ char bar(int i) {
+return x.data()[i];
+ }
+ private:
+ T x;
+};
+
+void f(int i) {
+ MyVector v;
+ int x = v.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call to .data() [readability-redundant-data-call]
+ // CHECK-FIXES: int x = v[i];
+
+ std::string s;
+ char c1 = s.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant call to .data()
+ // CHECK-FIXES: char c1 = s[i];
+
+ std::string_view sv;
+ char c2 = sv.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call to .data()
+ // CHECK-FIXES: char c2 = sv[i];
+
+ std::string* ps =
+ char c3 = ps->data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant call to .data()
+ // CHECK-FIXES: char c3 = (*ps)[i];
+
+ char c4 = (*ps).data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant call to .data()
+ // CHECK-FIXES: char c4 = (*ps)[i];
+
+ DO(char c5 = s.data()[i]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: redundant call to .data()
+ // CHECK-FIXES: DO(char c5 = s[i]);
+
+ char c6 = ACCESS(s).data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: redundant call to .data()
+ // CHECK-FIXES: char c6 = ACCESS(s)[i];
+
+ char c7 = ACCESS(s.data())[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+ // CHECK-FIXES: char c7 = ACCESS(s)[i];
+
+ char c8 = ACCESS(s.data()[i]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+ // CHECK-FIXES: char c8 = ACCESS(s[i]);
+
+ char c9 = GET(s, i);
+
+ char c10 = Foo{}.bar(i);
+}
Index: docs/clang-tidy/checks/readability-redundant-data-call.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-data-call.rst
@@ -0,0 +1,23 @@
+.. title:: clang-tidy - readability-redundant-data-call
+
+readability-redundant-data-call
+===
+
+This check finds and suggests removing redundant `.data()` calls.
+Currently this covers calling `.data()` and immediately doing array subscript
+operation to obtain a single element, in which case simply calling `operator[]`
+suffice.
+
+Examples:
+
+.. code-block:: c++
+ std::string s = ...;
+ char c = s.data()[i]; // char c = s[i];
+
+Options
+---
+
+.. option:: Types
+
+ The list of type(s) that triggers this check. Default covers `std::string`,
+ `std::string_view`, `std::vector`, `std::array`.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -91,8 +91,8 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
- fuchsia-header-anon-namespaces (redirects to google-build-namespaces)
fuchsia-default-arguments
+ fuchsia-header-anon-namespaces (redirects
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
aaron.ballman added a comment. Have you run this over any large code bases to see whether the check triggers in practice? Comment at: docs/clang-tidy/checks/list.rst:95 fuchsia-default-arguments + fuchsia-header-anon-namespaces (redirects to google-build-namespaces) fuchsia-multiple-inheritance This change looks unrelated to the patch and should be handled separately. Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:6 + +This check suggests removing redundant `.data()` calls. + What does it mean for a `data()` call to be redundant? I think the documentation needs to explain that a bit more. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:63 + + This check suggests removing redundant `.data()` calls. + Eugene.Zelenko wrote: > I would suggest //Finds redundant `.data()` calls.// Same in documentation. > > Please also move to new checks list in alphabetical order. Please enclose .data() in ``. Same in documentation. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
Eugene.Zelenko added a comment.
Please also take a look on 26817 for another idea for //.data()//.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:20
+namespace readability {
+namespace {
+const char kDefaultTypes[] =
Please separate with empty line.
Comment at: clang-tidy/readability/RedundantDataCallCheck.cpp:32
+void RedundantDataCallCheck::registerMatchers(MatchFinder *Finder) {
+ using ast_matchers::allOf;
+ using ast_matchers::anyOf;
Isn't this list redundant for //using namespace clang::ast_matchers//?
Comment at: docs/ReleaseNotes.rst:63
+
+ This check suggests removing redundant `.data()` calls.
+
I would suggest //Finds redundant `.data()` calls.// Same in documentation.
Please also move to new checks list in alphabetical order.
Comment at: docs/clang-tidy/checks/readability-redundant-data-call.rst:14
+
+The type(s) that triggers this check can be configured with `Types` option.
Please look on other check documentation for example of option descriptions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45702: [clang-tidy] Add a new check, readability-redundant-data-call, that finds and removes redundant calls to .data().
shuaiwang created this revision.
Herald added subscribers: cfe-commits, xazax.hun, mgorny, klimek.
shuaiwang added a reviewer: sbenza.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45702
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantDataCallCheck.cpp
clang-tidy/readability/RedundantDataCallCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-redundant-data-call.rst
test/clang-tidy/readability-redundant-data-call.cpp
Index: test/clang-tidy/readability-redundant-data-call.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-data-call.cpp
@@ -0,0 +1,108 @@
+// RUN: %check_clang_tidy %s readability-redundant-data-call %t \
+// RUN: -config="{CheckOptions: \
+// RUN: [{key: readability-redundant-data-call.Types, \
+// RUN: value: '::std::basic_string;::std::basic_string_view;MyVector'}]}" --
+
+namespace std {
+
+template
+class basic_string {
+ public:
+ using size_type = unsigned;
+ using value_type = T;
+ using reference = value_type&;
+ using const_reference = const value_type&;
+
+ reference operator[](size_type);
+ const_reference operator[](size_type) const;
+ T* data();
+ const T* data() const;
+};
+
+using string = basic_string;
+
+template
+class basic_string_view {
+ public:
+ using size_type = unsigned;
+ using const_reference = const T&;
+ using const_pointer = const T*;
+
+ constexpr const_reference operator[](size_type) const;
+ constexpr const_pointer data() const noexcept;
+};
+
+using string_view = basic_string_view;
+
+}
+
+template
+class MyVector {
+ public:
+ using size_type = unsigned;
+ using const_reference = const T&;
+ using const_pointer = const T*;
+
+ const_reference operator[](size_type) const;
+ const T* data() const noexcept;
+};
+
+#define DO(x) do { x; } while (false)
+#define ACCESS(x) (x)
+#define GET(x, i) (x).data()[i]
+
+template
+class Foo {
+ public:
+ char bar(int i) {
+return x.data()[i];
+ }
+ private:
+ T x;
+};
+
+void f(int i) {
+ MyVector v;
+ int x = v.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call to .data() [readability-redundant-data-call]
+ // CHECK-FIXES: int x = v[i];
+
+ std::string s;
+ char c1 = s.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant call to .data()
+ // CHECK-FIXES: char c1 = s[i];
+
+ std::string_view sv;
+ char c2 = sv.data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call to .data()
+ // CHECK-FIXES: char c2 = sv[i];
+
+ std::string* ps =
+ char c3 = ps->data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant call to .data()
+ // CHECK-FIXES: char c3 = (*ps)[i];
+
+ char c4 = (*ps).data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant call to .data()
+ // CHECK-FIXES: char c4 = (*ps)[i];
+
+ DO(char c5 = s.data()[i]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: redundant call to .data()
+ // CHECK-FIXES: DO(char c5 = s[i]);
+
+ char c6 = ACCESS(s).data()[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: redundant call to .data()
+ // CHECK-FIXES: char c6 = ACCESS(s)[i];
+
+ char c7 = ACCESS(s.data())[i];
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+ // CHECK-FIXES: char c7 = ACCESS(s)[i];
+
+ char c8 = ACCESS(s.data()[i]);
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: redundant call to .data()
+ // CHECK-FIXES: char c8 = ACCESS(s[i]);
+
+ char c9 = GET(s, i);
+
+ char c10 = Foo{}.bar(i);
+}
Index: docs/clang-tidy/checks/readability-redundant-data-call.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-data-call.rst
@@ -0,0 +1,14 @@
+.. title:: clang-tidy - readability-redundant-data-call
+
+readability-redundant-data-call
+===
+
+This check suggests removing redundant `.data()` calls.
+
+Examples:
+
+.. code-block:: c++
+ std::string s = ...;
+ char c = s.data()[i]; // char c = s[i];
+
+The type(s) that triggers this check can be configured with `Types` option.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -91,8 +91,8 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
- fuchsia-header-anon-namespaces (redirects to google-build-namespaces)
fuchsia-default-arguments
+ fuchsia-header-anon-namespaces (redirects to google-build-namespaces)
fuchsia-multiple-inheritance
fuchsia-overloaded-operator
fuchsia-statically-constructed-objects
@@ -218,6 +218,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+
