Page MenuHomePhabricator

[clang-tidy] Don't modernize-raw-string-literal if replacement is longer.
ClosedPublic

Authored by leanil on Jan 13 2017, 2:14 AM.

Details

Summary

modernize-raw-string-literal suggests to replace e.g.:

std::string s{ "foo \"" } with std::string s{R"(foo ")" };
std::string s{ ":)\"" }   with std::string s{R"lit(:)")lit" };

which are not necessarily improvements.
Maybe it should only suggest replacements, that are not longer than the original.

Fixes PR30964

Diff Detail

Repository
rL LLVM

Event Timeline

leanil updated this revision to Diff 84258.Jan 13 2017, 2:14 AM
leanil retitled this revision from to [clang-tidy] Don't modernize-raw-string-literal if replacement is longer..
leanil updated this object.
leanil added a project: Restricted Project.
leanil added a subscriber: cfe-commits.
fhahn added a subscriber: fhahn.Jan 14 2017, 9:41 AM
alexfh requested changes to this revision.Jan 14 2017, 2:19 PM
alexfh edited edge metadata.
alexfh added inline comments.
test/clang-tidy/modernize-raw-string-literal.cpp
94 ↗(On Diff #84258)

Does this test fail without the patch? Also, should there be a test covering the case where no replacement is made (and the current code issues a warning)?

This revision now requires changes to proceed.Jan 14 2017, 2:19 PM
leanil added inline comments.Jan 15 2017, 11:41 PM
test/clang-tidy/modernize-raw-string-literal.cpp
94 ↗(On Diff #84258)

No, it doesn't. This is to check that MeasureTokenLength works as I expect. It wouldn't warn if it measured the two parts separately.
And I left the check here for those who may have the same concerns, although it's indeed not the core functionality I test here. Should I remove it?

alexfh added inline comments.Jan 16 2017, 2:45 PM
test/clang-tidy/modernize-raw-string-literal.cpp
94 ↗(On Diff #84258)

The test is fine, but it's better to add it as a separate patch. This patch needs proper tests of the new functionality.

leanil updated this revision to Diff 84805.Jan 18 2017, 2:39 AM
leanil edited edge metadata.

Add test cases for the new functionality.
These should not produce warnings, because their raw string replacement would be longer.

Don't do this without introducing an option to turn it off.

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

I agree. Which mode do you think should be the default?

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

I agree. Which mode do you think should be the default?

As long as you can control it, I don't think it matters much and preference of the submitter is fine.

leanil updated this revision to Diff 85014.Jan 19 2017, 12:42 PM

Add config parameter to control new functionality.
Update the test cases accordingly.

This revision is now accepted and ready to land.Jan 24 2017, 7:17 AM
This revision was automatically updated to reflect the committed changes.
dkrupp added a subscriber: dkrupp.Apr 11 2017, 6:59 AM