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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
