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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Looks mostly good, a few nits.


================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:82
@@ +81,3 @@
+                        ? std::string{R"lit()")lit"}
+                        : (")" + Delimiter + R"(")")) != StringRef::npos;
+}
----------------
LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > This is a wonderful demonstration of why I hate raw string literals on shorter strings. I had to stare at that raw string for quite some time to figure it out. :-(
> I used a raw string literal here because that's what this check would have recommended on this code :-).
> 
> However, your feedback is useful.  If I subsequently enhance this check with a parameter that says the minimum length a string literal must have before it is to be transformed into a raw string literal and the default value for that parameter is something like 5, would that address your concern?
That (a way to configure some thresholds of "sensitivity" of this check, so it doesn't change `"\""` to `R"(")"` etc.) is roughly what I suggested a while ago, and we decided that this can be addressed in a follow up.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:98
@@ +97,3 @@
+}
+
+} // namespace
----------------
> 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.

The other concern is:

>> 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).

However, given the low probability of this scenario, I don't want to waste more of our time on this. The current algorithm is good enough for the first version.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:104
@@ +103,3 @@
+    : ClangTidyCheck(Name, Context),
+      DelimiterStem{Options.get("DelimiterStem", "lit")} {}
+
----------------
AFAIU, braced initializers in constructor initializer lists are not supported on MSVC 2013.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:104
@@ +103,3 @@
+    : ClangTidyCheck(Name, Context),
+      DelimiterStem{Options.get("DelimiterStem", "lit")} {}
+
----------------
alexfh wrote:
> AFAIU, braced initializers in constructor initializer lists are not supported on MSVC 2013.
> Then the documentation needs to be updated. (Actually I couldn't find any 
> documentation on these flags and how they relate to each other because they 
> use preprocessor hell to generate bitfields in a structure.)

As with most things in open source, the documentation is missing, since nobody yet needed it strongly enough to write it.

If you feel it's worth your time, send a patch to whoever maintains Clang frontend code these days. The documentation should go somewhere around include/clang/Frontend/LangStandards.def:115 or include/clang/Basic/LangOptions.def:77.


http://reviews.llvm.org/D16529