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

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:97
@@ +96,3 @@
+}
+
+} // namespace
----------------
alexfh wrote:
> > I believe we agree on the following: ...
> 
> Yes.
> 
> > Where we seem to disagree is what algorithm should be used to determine the 
> > delimiter when a delimited raw string literal is required.
> 
> Pretty much so. IMO, we don't need a universal algorithm that will hardly ever go further than the second iteration, and even in this case would produce a result that's likely undesirable (as I said, `R"lit0(....)lit0"` is not what I would want to see in my code).
> 
> The possibility of this code causing performance issues is probably not that high, but I can imagine a code where this could be sub-optimal.
> 
> > My implementation is the minimum performance impact for the typical case 
> > where the string does not contain )".
> 
> One concern about this part is that it could issue 4 concatenation calls fewer in case of an empty delimiter. This may already be handled well by the `llvm::Twine` though.
> 
> Any code following potentially-problematic patterns might be fine on its own, but it will attract unnecessary attention when reading code. High frequency of such false alarms has non-trivial cost for code readers and makes it harder to find actual problems.
> 
> So please, change the code to avoid these issues. Here's a possible alternative:
> 
>   llvm::Optional<std::string> asRawStringLiteral(const StringLiteral *Literal) {
>     const StringRef Bytes = Literal->getBytes();
>     static const StringRef Delimiters[2][] =
>       {{"R\"(", ")\""}, {"R\"lit(", ")lit\""}, {"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[1]) != StringRef::npos)
>         return (Delim[0] + Bytes + Delim[1]).str();
>     }
>     // Give up gracefully on literals having all our delimiters.
>     return llvm::None;
>   }
I just don't see why replacing a general algorithm that always works with a bunch of hard-coded special cases is better.

You've reiterated your preference for one over the other, but since it simply a matter of preference, I don't see why this should be a requirement.

Working with the hard-coded list of delimiters is no more or less efficient than the general algorithm that I implemented, so efficiency is not the concern here.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:110
@@ +109,3 @@
+  if (const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("lit")) {
+    if (isMacroExpansionLocation(Result, Literal->getLocStart()))
+      return;
----------------
alexfh wrote:
>   if (Literal->getLocStart().isMacroID())
>     return;
I don't understand what this code is doing.  `isMacroID` is undocumented, so I don't know what it means for this function to return true without reverse engineering the implementation.


http://reviews.llvm.org/D16529