[clang-tools-extra] [clang-tidy] Add readability-string-view-substr check (PR #120055)
Helmut Januschka via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 17 02:01:31 PST 2025
hjanuschka wrote:
@PiotrZSL feedback addressed
@5chmidti - thank you for your feedback, i have now invested multiple days over a few weeks to somehow make this work.
started multiple attempts from scratch, at this point i don't even know what i have tried and what not, the last commit.
https://github.com/llvm/llvm-project/pull/120055/commits/173688401ea849379ddfd62def3504333b40cac1
seems to produce the right result, yet llvm-lit fails, and i have no clou.
<details>
<summary>Test Output</summary>
```
-- Testing: 1 tests, 1 workers --
FAIL: Clang Tools :: clang-tidy/checkers/readability/stringview_substr.cpp (1 of 1)
******************** TEST 'Clang Tools :: clang-tidy/checkers/readability/stringview_substr.cpp' FAILED ********************
Exit Code: 1
Command Output (stdout):
--
Running ['clang-tidy', '/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp', '-fix', '--checks=-*,readability-stringview-substr', '--config={}', '--', '-std=c++17', '-nostdinc++']...
------------------------ clang-tidy output -----------------------
17 warnings generated.
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:28:3: warning: prefer 'remove_prefix' over 'substr' for removing characters from the start [readability-stringview-substr]
28 | sv = sv.substr(5);
| ^~~~~~~~~~~~~~~~~
| sv.remove_prefix(5)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:28:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:33:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
33 | sv = sv.substr(0, sv.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:33:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:38:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
38 | sv = sv.substr(0, sv.length() - (3 + 2));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix((3 + 2))
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:38:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:48:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
48 | sv = sv.substr(0, sv.size() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:48:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:53:3: warning: redundant self-copy [readability-stringview-substr]
53 | sv = sv.substr(0, sv.size());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:53:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:56:3: warning: redundant self-copy [readability-stringview-substr]
56 | sv = sv.substr(0, sv.size() - 0);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:56:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:60:3: warning: prefer direct copy over substr [readability-stringview-substr]
60 | sv2 = sv.substr(0, sv.size());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv2 = sv
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:60:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:88:3: warning: redundant self-copy [readability-stringview-substr]
88 | sv = sv.substr(0, sv.length());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:88:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:91:3: warning: redundant self-copy [readability-stringview-substr]
91 | sv = sv.substr(0, sv.length() - 0);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:91:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:95:3: warning: prefer direct copy over substr [readability-stringview-substr]
95 | sv1 = sv.substr(0, sv.length());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv1 = sv
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:95:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:99:3: warning: prefer direct copy over substr [readability-stringview-substr]
99 | sv2 = sv.substr(0, sv.length() - 0);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv2 = sv
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:99:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:111:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
111 | sv = sv.substr(kZero, sv.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:111:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:119:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
119 | sv = sv.substr(START_POS, sv.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:119:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:123:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
123 | sv = sv.substr((0), sv.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:123:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:127:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
127 | sv = sv.substr(0u, sv.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:127:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:131:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
131 | sv = sv.substr(0UL, sv.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| sv.remove_suffix(3)
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:131:3: note: FIX-IT applied suggested code changes
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:145:3: warning: prefer assignment and remove_suffix over substr [readability-stringview-substr]
145 | sv = sv2.substr(0, sv2.length() - 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:145:3: note: FIX-IT applied suggested code changes
clang-tidy applied 17 of 17 suggested fixes.
------------------------------------------------------------------
diff -u /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.orig /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp failed:
--- /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.orig 2025-01-17 10:58:28.139518728 +0100
+++ /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp 2025-01-17 10:58:28.151518945 +0100
@@ -25,17 +25,17 @@
std::string_view sv2("test");
// Should match: remove_prefix
- sv = sv.substr(5);
+ sv.remove_prefix(5);
//
//
// Should match: remove_suffix
- sv = sv.substr(0, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
// Should match: remove_suffix with complex expression
- sv = sv.substr(0, sv.length() - (3 + 2));
+ sv.remove_suffix((3 + 2));
//
//
}
@@ -45,19 +45,19 @@
std::string_view sv2("test");
// Should match: remove_suffix with size()
- sv = sv.substr(0, sv.size() - 3);
+ sv.remove_suffix(3);
//
//
// Should match: remove redundant self copies
- sv = sv.substr(0, sv.size());
+
//
- sv = sv.substr(0, sv.size() - 0);
+
//
// Should match: simplify copies between different variables
- sv2 = sv.substr(0, sv.size());
+ sv2 = sv;
//
//
}
@@ -85,18 +85,18 @@
std::string_view sv2("test");
// Should match: remove redundant self copies
- sv = sv.substr(0, sv.length());
+
//
- sv = sv.substr(0, sv.length() - 0);
+
//
// Should match: simplify copies between different variables
- sv1 = sv.substr(0, sv.length());
+ sv1 = sv;
//
//
- sv2 = sv.substr(0, sv.length() - 0);
+ sv2 = sv;
//
//
}
@@ -108,7 +108,7 @@
#define START_POS 0
// Should match: various forms of zero in first argument
- sv = sv.substr(kZero, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
@@ -116,19 +116,19 @@
//
//
- sv = sv.substr(START_POS, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
- sv = sv.substr((0), sv.length() - 3);
+ sv.remove_suffix(3);
//
//
- sv = sv.substr(0u, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
- sv = sv.substr(0UL, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
}
@@ -142,7 +142,8 @@
sv = sv.substr(1, sv.length() - 3); // No warning
// Should match: Different string_views with remove_suffix pattern
- sv = sv2.substr(0, sv2.length() - 3);
+ sv = sv2;
+ sv.remove_suffix(3);
//
//
//
------------------------------ Fixes -----------------------------
--- /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.orig 2025-01-17 10:58:28.139518728 +0100
+++ /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp 2025-01-17 10:58:28.151518945 +0100
@@ -25,17 +25,17 @@
std::string_view sv2("test");
// Should match: remove_prefix
- sv = sv.substr(5);
+ sv.remove_prefix(5);
//
//
// Should match: remove_suffix
- sv = sv.substr(0, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
// Should match: remove_suffix with complex expression
- sv = sv.substr(0, sv.length() - (3 + 2));
+ sv.remove_suffix((3 + 2));
//
//
}
@@ -45,19 +45,19 @@
std::string_view sv2("test");
// Should match: remove_suffix with size()
- sv = sv.substr(0, sv.size() - 3);
+ sv.remove_suffix(3);
//
//
// Should match: remove redundant self copies
- sv = sv.substr(0, sv.size());
+
//
- sv = sv.substr(0, sv.size() - 0);
+
//
// Should match: simplify copies between different variables
- sv2 = sv.substr(0, sv.size());
+ sv2 = sv;
//
//
}
@@ -85,18 +85,18 @@
std::string_view sv2("test");
// Should match: remove redundant self copies
- sv = sv.substr(0, sv.length());
+
//
- sv = sv.substr(0, sv.length() - 0);
+
//
// Should match: simplify copies between different variables
- sv1 = sv.substr(0, sv.length());
+ sv1 = sv;
//
//
- sv2 = sv.substr(0, sv.length() - 0);
+ sv2 = sv;
//
//
}
@@ -108,7 +108,7 @@
#define START_POS 0
// Should match: various forms of zero in first argument
- sv = sv.substr(kZero, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
@@ -116,19 +116,19 @@
//
//
- sv = sv.substr(START_POS, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
- sv = sv.substr((0), sv.length() - 3);
+ sv.remove_suffix(3);
//
//
- sv = sv.substr(0u, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
- sv = sv.substr(0UL, sv.length() - 3);
+ sv.remove_suffix(3);
//
//
}
@@ -142,7 +142,8 @@
sv = sv.substr(1, sv.length() - 3); // No warning
// Should match: Different string_views with remove_suffix pattern
- sv = sv2.substr(0, sv2.length() - 3);
+ sv = sv2;
+ sv.remove_suffix(3);
//
//
//
------------------------------------------------------------------
FileCheck -input-file=/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp /home/chrome/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp -check-prefixes=CHECK-FIXES -strict-whitespace failed:
/home/chrome/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp:147:19: error: CHECK-FIXES: expected string not found in input
// CHECK-FIXES: sv = sv2;
^
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:146:22: note: scanning from here
sv.remove_suffix(3);
^
/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp:155:20: note: possible intended match here
const auto copy = sv = sv.substr(0, sv.length() - 3); // No warning
^
Input file: /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp
Check file: /home/chrome/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
-dump-input=help explains the following input dump.
Input was:
<<<<<<
.
.
.
141: // No match: substr used for prefix or mid-view
142: sv = sv.substr(1, sv.length() - 3); // No warning
143:
144: // Should match: Different string_views with remove_suffix pattern
145: sv = sv2;
146: sv.remove_suffix(3);
check:147'0 X~ error: no match found
147: //
check:147'0 ~~~~~
148: //
check:147'0 ~~~~~
149: //
check:147'0 ~~~~~
150: }
check:147'0 ~~
151:
check:147'0 ~
152: void f(std::string_view) {}
check:147'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
153: void test_expr_with_cleanups() {
check:147'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
154: std::string_view sv("test");
check:147'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
155: const auto copy = sv = sv.substr(0, sv.length() - 3); // No warning
check:147'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:147'1 ? possible intended match
156: f(sv = sv.substr(0, sv.length() - 3)); // No warning
check:147'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
157: }
check:147'0 ~~
158:
check:147'0 ~
>>>>>>
--
Command Output (stderr):
--
RUN: at line 1: /usr/bin/python3 /home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /home/chrome/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp readability-stringview-substr -std=c++17 /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp
+ /usr/bin/python3 /home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /home/chrome/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp readability-stringview-substr -std=c++17 /home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp
Traceback (most recent call last):
File "/home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py", line 388, in <module>
main()
File "/home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py", line 384, in main
CheckRunner(args, extra_args).run()
File "/home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py", line 299, in run
self.check_fixes()
File "/home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py", line 247, in check_fixes
try_run(
File "/home/chrome/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py", line 61, in try_run
process_output = subprocess.check_output(args, stderr=subprocess.STDOUT).decode(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 466, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/subprocess.py", line 571, in run
raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['FileCheck', '-input-file=/home/chrome/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/stringview_substr.cpp.tmp.cpp', '/home/chrome/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp', '-check-prefixes=CHECK-FIXES', '-strict-whitespace']' returned non-zero exit status 1.
--
********************
********************
Failed Tests (1):
Clang Tools :: clang-tidy/checkers/readability/stringview_substr.cpp
Testing Time: 0.14s
Total Discovered Tests: 1
Failed: 1 (100.00%)
```
</details>
in the diff that shows on top of the output it "feels" like it is ok, but the multiline replacement is not confirmed working.
so i am here to ask for help.
- any ideas how to fix the test file?
- in general is the approach correctly understood as you mentioned it.
overall - at the end this would require a full re-review.
https://github.com/llvm/llvm-project/pull/120055
More information about the cfe-commits
mailing list