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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for addressing (most of) the comments!

I have a few more comments and the comment about the implementation of `asRawStringLiteral` still applies.

Also, please remove unrelated files from the patch.


================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:68
@@ +67,3 @@
+  // The NUL character disqualifies this literal.
+  if (Bytes.find_first_of(StringRef("\000", 1)) != StringRef::npos)
+    return false;
----------------
A `.find('\0')` should be enough, no need for `.find_first_of()`.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:78
@@ +77,3 @@
+  // Already a raw string literal if R comes before ".
+  if (Text.find('R') < Text.find('"'))
+    return false;
----------------
I think 'R' in string literals can't be anywhere but at the very beginning. So I'd prefer `Text.front() == 'R'` as a simpler and a more straightforward alternative.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:86
@@ +85,3 @@
+
+  return containsEscapes(Text, R"lit('\"?x01)lit");
+}
----------------
What's the reason to make the code longer and less readable by usingb `lit` here? 

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:115
@@ +114,3 @@
+  if (const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit")) {
+    if (containsEscapedCharacters(Result, Literal))
+      replaceWithRawStringLiteral(Result, Literal);
----------------
This is purely a matter of preference, but I personally don't find that, for example, `R"(\)"` is better that `"\\"`, and in general, that removing one backslash from a string literal is worth adding three more special characters (`R`, `(` and `)`). I think, the check should either be more conservative (e.g. leave alone string literals that don't become shorter, when rewritten as raw string literals) or be configurable in this regard. Another concern is that replacing a large number of `\n` sequences with actual newlines might make the code less readable (also a matter of preference, I guess). This also deserves either a cleverer analysis to prevent undesirable replacements and/or a separate configuration option.

================
Comment at: docs/clang-tidy/checks/modernize-raw-string-literal.rst:45
@@ +44,3 @@
+Trailing whitespace at the end of a source code line is not visible in an
+editor.  Trailing whitespace is likely to be stripped by editors and other
+tools, changing the meaning of the literal.
----------------
I'm not a native speaker, but it seems that the rule to use two spaces after a period is outdated. Also, a quick search shows that in LLVM/Clang documentation a majority of files use exclusively one space after a period. That said, I don't feel strongly about that and this comment is just FYI.


http://reviews.llvm.org/D16529