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