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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:27
@@ +26,3 @@
+  // we only transform ASCII string literals
+  if (!Literal->isAscii())
+    return false;
----------------
Why can't the check convert non-ascii string literals to corresponding raw string literals?

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:57
@@ +56,3 @@
+
+  // already a raw string literal if R comes before "
+  if (Text.find_first_of("R") < Text.find_first_of(R"(")"))
----------------
nit: Capitalization, punctuation. Same for other comments in the file.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:58
@@ +57,3 @@
+  // already a raw string literal if R comes before "
+  if (Text.find_first_of("R") < Text.find_first_of(R"(")"))
+    return false;
----------------
aaron.ballman wrote:
> I think it might make more sense to block on D16012 (which will hopefully be reviewed relatively soon), and then just use the StringLiteral object to specify whether it's a raw string literal or not.
Also, why `find_first_of` instead of just `find(char)`? It will allow you to get rid of the `R"(")"` awfulness ;)

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:61
@@ +60,3 @@
+
+  const bool HasBackSlash = Text.find(R"(\\)") != StringRef::npos;
+  const bool HasNewLine = Text.find(R"(\n)") != StringRef::npos;
----------------
I'd remove these variables and either just combine all these to a single condition (which would make the code benefit from short-circuiting) or just use a regular expression, which can be more effective than N lookups in a row (not sure for which N though). Another benefit of the regular expression is reduction of the number of `R"(`s in the code: `R"(\\[n'"?\\])"` ;).

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:77
@@ +76,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+    if (Counter == 0) {
----------------
Why don't you try an empty delimiter? It will cover a majority of cases.

Also, even though the function is used only when generating fix-it hints, it still should be more effective than it is now. Most of the heap allocations and copies caused by the usage of std::ostringstream, std::string and std::string::operator+ can be avoided, e.g. like this:

  static const StringRef Delimiters[2][] =
    {{"", "R\"(", ")\""}, {"lit", "R\"lit(", ")lit\""}, {"str", "R\"str(", ")str\""},
    /* add more different ones to make it unlikely to meet all of these in a single literal in the wild */};
  for (const auto &Delim : Delimiters) {
    if (Bytes.find(Delim[2]) != StringRef::npos)
      return (Delim[1] + Bytes + Delim[2]).str();
  }
  // Give up gracefully on literals having all our delimiters.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:107
@@ +106,3 @@
+  if (const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit"))
+    preferRawStringLiterals(Result, Literal);
+}
----------------
I don't see a reason to have a separate function, I'd just inline it here.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:113
@@ +112,3 @@
+  if (containsEscapedCharacters(Result, Literal)) {
+    SourceRange ReplacementRange = Literal->getSourceRange();
+    CharSourceRange CharRange = Lexer::makeFileCharRange(
----------------
I'd just use the expression instead of the variable.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:117
@@ +116,3 @@
+        Result.Context->getLangOpts());
+    StringRef Replacement = asRawStringLiteral(Literal);
+    diag(Literal->getLocStart(),
----------------
`Replacement` will point to deleted memory right after this line.


http://reviews.llvm.org/D16529