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