[PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

There are still a few comments open. One more important thing is to try running this check over a large enough project (LLVM + Clang, for example), apply fixes, look at the results and try to build the fixed code and run all tests. You can use the clang-tidy/tool/run-clang-tidy.py script (with proper -clang-tidy-binary and -clang-apply-replacements-binary options) to run the check and apply the fixes.


================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:68
@@ +67,3 @@
+  // The NUL character disqualifies this literal.
+  if (Bytes.find('\000') != StringRef::npos)
+    return false;
----------------
Do you have a test for this?

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:96
@@ +95,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+    Delimiter = (Counter == 0) ? "lit" : "lit" + std::to_string(Counter);
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > Please address my comment above that relates to this code. Specifically, I find this generic algorithm unnecessarily inefficient and too rigid, i.e. one can't configure a custom set of delimiters that don't follow the <prefix><number> pattern. I suggest using a list of pre-concatenated pairs of strings (like `R"lit(` and `)lit"`).
> Raw strings are new and not many people are using them because the don't have a good way to automatically convert disgusting quoted strings to raw strings.  So I don't think it is reasonable to draw conclusions by looking in existing code bases for raw strings.
> 
> We're having the same conversation we've had before.  I'm trying to do a basic check and get things working properly and the review comments are tending towards a desire for some kind of perfection.
> 
> I don't see why we have to make the perfect the enemy of the good.  Nothing you are suggesting **must** be present now in order for this check to function properly and reasonably.
We're looking at this from different perspectives. From my point of view, preventing a potentially wasteful code in ClangTidy checks before it's even committed is much easier than tracking down performance issues when tens of checks run on multiple machines analyzing millions lines of code. So I'm asking to avoid writing code using string allocations and concatenations when there are good alternatives. Apart from possible performance issues, in this case the generic solution you suggest is targets extremely rare cases, while most real-world situations can be handled with a fixed set of delimiters (possibly, configured by the user, while I expect the preferences for the choice of delimiters to be very different in different teams).

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:116
@@ +115,3 @@
+    if (containsEscapedCharacters(Result, Literal))
+      replaceWithRawStringLiteral(Result, Literal);
+  }
----------------
aaron.ballman wrote:
> A configuration option makes sense to me, but I would be fine if it was a follow-on patch. I think that someone running this check is likely fine with the level of noise it will produce (which should be moderately low).
Agreed. This can be done in a follow up.

================
Comment at: test/clang-tidy/modernize-raw-string-literal.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
----------------
As usual, please add tests ensuring that replacements are done correctly in templates with multiple instantiations and in macro arguments (but not in macro bodies), e.g.

  template<typename T>
  void f() {
    (void)"asdf\\\\\\";
    // CHECK-FIXES: void f() {
    // CHECK-FIXES-NEXT: {{^  }}(void)R"(asdf\\\)";{{$}}
    // CHECK-FIXES-NEXT: {{^}$}}
  }
  void g() {
    f<int>();
    f<double>();
  }

  #define M(x) x x x "a\\b\\"
  void h() {
    const char *const s = M("c\\d\\");
    // CHECK-FIXES: {{^}}#define M(x) x x x "a\\b\\"{{$}}
    // CHECK-FIXES: {{^  }}const char *const s = M(R"(c\d\)");
  }




http://reviews.llvm.org/D16529