[PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 1 08:34:16 PST 2016
alexfh added inline comments.
================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:80
@@ +79,3 @@
+ return false;
+
+ const size_t NewLinePos = Text.find(R"(\n)");
----------------
aaron.ballman wrote:
> This is why I would still prefer to block on fixing StringLiteral. This is functional, but really kind of nasty -- in the common case (not already a raw string literal), this does two linear searches through the entire string literal.
Richard, you're right, it's `u8R"(...)"`, not `Ru8"(...)"` or something. Nevertheless, doing a full scan for `R` (the search for `"` wouldn't result in a full scan) in this condition shouldn't be necessary, it could first find a `"` and then check if the previous character is `R`. It would be a bit more code, but less needless work at runtime.
Alternatively, D16012 might be a better answer, if it ever makes it into Clang.
================
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:88
@@ +87,3 @@
+}
+
+bool containsDelimiter(StringRef Bytes, const std::string &Delimiter) {
----------------
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.
================
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);
----------------
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"`).
http://reviews.llvm.org/D16529
More information about the cfe-commits
mailing list