LegalizeAdulthood updated this revision to Diff 51738.
LegalizeAdulthood added a comment.
Remove brace initializers from test code.
http://reviews.llvm.org/D16529
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
LegalizeAdulthood updated this revision to Diff 51727.
LegalizeAdulthood added a comment.
Update release notes
http://reviews.llvm.org/D16529
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/RawStringLiteralCheck.cpp
aaron.ballman added a comment.
When running the tests from this patch locally (Win10, MSVC 2015, debug, x64),
I get:
1>-- Build started: Project: intrinsics_gen, Configuration: Debug x64 --
2>-- Build started: Project: ClangDiagnosticLex, Configuration: Debug x64
--
3>--
LegalizeAdulthood updated this revision to Diff 51105.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.
Update from comments.
Update documentation to reflect changes in the implementation.
I do not have commit access.
Patch by Richard Thomson
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 added a comment.
I do not have commit access, so I will need someone to commit this for me.
Patch by Richard Thomson
thanks.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:82
@@ +81,3 @@
+? std::string{R"lit()")lit"}
+
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
In http://reviews.llvm.org/D16529#366433, @LegalizeAdulthood wrote:
> Squeak. This has been waiting on review for over two weeks
Sorry for the delay, I was at WG21
LegalizeAdulthood added a comment.
Squeak. This has been waiting on review for over two weeks
http://reviews.llvm.org/D16529
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
LegalizeAdulthood updated this revision to Diff 47927.
LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added a comment.
Update from review comments.
Avoid some string concatenation when delimiter is empty.
http://reviews.llvm.org/D16529
Files:
LegalizeAdulthood updated this revision to Diff 47929.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.
Add FIXME for macro argument test case.
http://reviews.llvm.org/D16529
Files:
clang-tidy/modernize/CMakeLists.txt
alexfh added a comment.
Sorry for the delay. I'm trying to prioritize reviews that are taking less time
per-iteration. Unfortunately, here we have a bunch of disagreements and I have
to spend significant time to read through your arguments and address your
points.
Comment
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
LegalizeAdulthood added a comment.
Squeak?
http://reviews.llvm.org/D16529
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
LegalizeAdulthood added a comment.
I wrote:
> Repeated searching of the string for the literal delimiter could be avoided
> by changing the routine to search for the delimiter. If present, it could
> examine the characters following the literal to make the literal more unique
> and then
LegalizeAdulthood marked 5 inline comments as done.
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" +
alexfh added a comment.
There are still a few comments open. One more important thing is to try running
this check over a large enough project (LLVM + Clang, for example), apply
fixes, look at the results and try to build the fixed code and run all tests.
You can use the
LegalizeAdulthood added a comment.
I agree it needs more testing.
I think also my current approach to newlines is overly aggressive and will
result in more raw string literals than people would prefer.
It's really the Windows style path separators and regex ones that are not
controversial
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.
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:80
@@ +79,3 @@
+return false;
+
+ const size_t NewLinePos = Text.find(R"(\n)");
This is why I would still prefer to block on fixing StringLiteral. This is
LegalizeAdulthood added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:88
@@ +87,3 @@
+}
+
+bool containsDelimiter(StringRef Bytes, const std::string ) {
alexfh wrote:
> aaron.ballman wrote:
> > I think Alex's point is: why not
LegalizeAdulthood updated this revision to Diff 46504.
LegalizeAdulthood added a comment.
Update from comments
http://reviews.llvm.org/D16529
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/RawStringLiteralCheck.cpp
LegalizeAdulthood added inline comments.
Comment at: docs/clang-tidy/checks/modernize-raw-string-literal.rst:46
@@ +45,3 @@
+editor. Trailing whitespace is likely to be stripped by editors and other
+tools, changing the meaning of the literal.
+
LegalizeAdulthood marked an inline comment as done.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:79
@@ +78,3 @@
+ if (Text.find('R') < Text.find('"'))
+return false;
+
Take a look at http://en.cppreference.com/w/cpp/language/string_literal
alexfh added a comment.
Thank you for addressing (most of) the comments!
I have a few more comments and the comment about the implementation of
`asRawStringLiteral` still applies.
Also, please remove unrelated files from the patch.
Comment at:
alexfh added a comment.
In http://reviews.llvm.org/D16529#340240, @alexfh wrote:
> Also, please remove unrelated files from the patch.
Please ignore this part, I missed the new diff.
http://reviews.llvm.org/D16529
___
cfe-commits mailing list
bkramer added a subscriber: bkramer.
bkramer added a comment.
Why are you re-adding all those Makefiles?
http://reviews.llvm.org/D16529
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
LegalizeAdulthood added a comment.
In http://reviews.llvm.org/D16529#339192, @bkramer wrote:
> Why are you re-adding all those Makefiles?
Ugh, I didn't even notice they were in there. It must have errantly slipped in
from rebasing.
http://reviews.llvm.org/D16529
LegalizeAdulthood updated this revision to Diff 46333.
LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added a comment.
Herald added a subscriber: klimek.
Update from review comments.
Extend analysis of newlines to prevent pathological raw strings from being
introduced.
alexfh added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:27
@@ +26,3 @@
+ // we only transform ASCII string literals
+ if (!Literal->isAscii())
+return false;
LegalizeAdulthood wrote:
> alexfh wrote:
> > Why can't the check
LegalizeAdulthood added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:107
@@ +106,3 @@
+ if (const auto *Literal = Result.Nodes.getNodeAs("lit"))
+preferRawStringLiterals(Result, Literal);
+}
alexfh wrote:
> I don't see a reason
LegalizeAdulthood added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:27
@@ +26,3 @@
+ // we only transform ASCII string literals
+ if (!Literal->isAscii())
+return false;
alexfh wrote:
> Why can't the check convert non-ascii
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:57
@@ +56,3 @@
+
+ // already a raw string literal if R comes before "
+ if (Text.find_first_of("R") < Text.find_first_of(R"(")"))
LegalizeAdulthood wrote:
> alexfh
alexfh added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:27
@@ +26,3 @@
+ // we only transform ASCII string literals
+ if (!Literal->isAscii())
+return false;
Why can't the check convert non-ascii string literals to
LegalizeAdulthood updated this revision to Diff 46101.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.
Update from comments:
- leave existing raw string literals unchanged
- don't change UTF-8, UTF-16, UTF-32 or wide character string literals
- apply
LegalizeAdulthood added inline comments.
Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:35
@@ +34,3 @@
+ const bool HasBackSlash = Text.find(R"(\\)") != StringRef::npos;
+ const bool HasNewLine = Text.find(R"(\n)") != StringRef::npos;
+ const bool HasQuote =
LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.
This check examines string literals and looks for those that could be more
directly expressed as a raw string literal.
Example:
```
char const *const
36 matches
Mail list logo
