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

Richard via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 1 09:08:20 PST 2016


LegalizeAdulthood added inline comments.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:88
@@ +87,3 @@
+}
+
+bool containsDelimiter(StringRef Bytes, const std::string &Delimiter) {
----------------
alexfh wrote:
> aaron.ballman wrote:
> > I think Alex's point is: why not R"('\"?x01)" (removing the need for lit)?
> Exactly, I was only talking about `lit`, not about using the raw string literal.
It was looking a little busy to my eyes with the raw string " and the quoted " close together.  It isn't necessary, but IMO improves readability.

================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:96
@@ +95,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+    Delimiter = (Counter == 0) ? "lit" : "lit" + std::to_string(Counter);
----------------
alexfh wrote:
> Please address my comment above that relates to this code. Specifically, I find this generic algorithm unnecessarily inefficient and too rigid, i.e. one can't configure a custom set of delimiters that don't follow the <prefix><number> pattern. I suggest using a list of pre-concatenated pairs of strings (like `R"lit(` and `)lit"`).
Raw strings are new and not many people are using them because the don't have a good way to automatically convert disgusting quoted strings to raw strings.  So I don't think it is reasonable to draw conclusions by looking in existing code bases for raw strings.

We're having the same conversation we've had before.  I'm trying to do a basic check and get things working properly and the review comments are tending towards a desire for some kind of perfection.

I don't see why we have to make the perfect the enemy of the good.  Nothing you are suggesting **must** be present now in order for this check to function properly and reasonably.


http://reviews.llvm.org/D16529





More information about the cfe-commits mailing list