Page MenuHomePhabricator

[clang-tidy]: modernize-use-default-member-init: Remove trailing comma and colon.
Needs RevisionPublic

Authored by jdemeule on Feb 19 2018, 11:37 PM.

Details

Summary

'modernize-user-default-member-init' does not automatically ask to remove comma and colon when replacements are produced.

It seems, when they are apply directly from clang-tidy, the RefactoringTool engine is smart enough to remove trailing tokens.
However, when fixes are exported, clang-apply-replacements cannot do the same trick and will lead trailing commas and colon as reported on https://bugs.llvm.org/show_bug.cgi?id=35051.

This patch made “modernize-use-default-member-init” generate explicitly the removal of these locations to made it work correctly in both case (-fix and -export-fixes).

Diff Detail

Event Timeline

jdemeule created this revision.Feb 19 2018, 11:37 PM
Eugene.Zelenko added inline comments.
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
15 ↗(On Diff #135012)

Please place it on a top, since header belong to project.

26 ↗(On Diff #135012)

Please use while (true) instead.

48 ↗(On Diff #135012)

Unnecessary curvy brackets.

177 ↗(On Diff #135012)

Unnecessary initializer. See readability-redundant-member-init.

271 ↗(On Diff #135012)

Unnecessary curvy brackets.

298 ↗(On Diff #135012)

Unnecessary curvy brackets.

302 ↗(On Diff #135012)

Unnecessary curvy brackets.

308 ↗(On Diff #135012)

Unnecessary curvy brackets.

317 ↗(On Diff #135012)

Unnecessary curvy brackets.

unittests/clang-tidy/ModernizerModuleTest.cpp
63

Please separate function from namespaces ends with empty line.

Eugene.Zelenko retitled this revision from modernize-use-default-member-init: Remove trailing comma and colon. to [clang-tidy]: modernize-use-default-member-init: Remove trailing comma and colon..Feb 20 2018, 10:42 AM
Eugene.Zelenko added a project: Restricted Project.

It seems, when they are apply directly from clang-tidy, the RefactoringTool engine is smart enough to remove trailing tokens. However, when fixes are exported, clang-apply-replacements cannot do the same trick and will lead trailing commas and colon

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
26 ↗(On Diff #135012)

Better still, don't use an infinite loop.

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

I was not aware of cleanupAroundReplacements. It should be a better option than fixing every check one by one. I am working on adding it on clang-apply-replacement for now and another review will be propose soon.

Should we discard this patch or keep only the added tests (if you found them relevant after fixing the comments)?

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

I was not aware of cleanupAroundReplacements. It should be a better option than fixing every check one by one. I am working on adding it on clang-apply-replacement for now and another review will be propose soon.

Should we discard this patch or keep only the added tests (if you found them relevant after fixing the comments)?

I'd keep the test cases, as they're good examples for us to ensure we don't regress.

It seems, when they are apply directly from clang-tidy, the RefactoringTool engine is smart enough to remove trailing tokens. However, when fixes are exported, clang-apply-replacements cannot do the same trick and will lead trailing commas and colon

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

Eric was going to look at this, IIRC.

Is there a way to make clang-apply-replacements smarter rather than forcing every check to jump through hoops? I'm worried that if we have to fix individual checks we'll just run into the same bug later.

See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161017/174238.html

I was not aware of cleanupAroundReplacements. It should be a better option than fixing every check one by one. I am working on adding it on clang-apply-replacement for now and another review will be propose soon.

That would be awesome Jeremy! Thanks!

I think it might be easier if you convert all replacements to tooling::AtomicChange and use applyAtomicChanges (https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/AtomicChange.h#L172) in clang-apply-replacements.

Let me know if you have any question, and I'm happy to review the patch when it's ready!

jdemeule updated this revision to Diff 135895.Feb 26 2018, 6:51 AM

As discuss, I have removed the modifications but keep the tests.
To have tests with meaning, I have also add a way to format code on 'runCheckOnCode'.

aaron.ballman added inline comments.Feb 26 2018, 9:32 AM
unittests/clang-tidy/ClangTidyTest.h
174

Extraneous whitespace.

176

Please don't use auto unless the type is explicitly spelled out in the initialization. (Here and elsewhere in the patch.)

188–189

This smells like it should be an assert rather than an unreachable.

196

Are you intending to implement this fixme in this patch?

jdemeule added inline comments.Feb 26 2018, 2:09 PM
unittests/clang-tidy/ClangTidyTest.h
188–189

Is returning an empty string OK?
This should let the assertion on the test do the job.

196

I suggest to remove the copy/paste containing the fixme. Is-it OK?
Maybe with some hints I can suggest a fix for this later.

aaron.ballman added inline comments.Feb 27 2018, 7:29 AM
unittests/clang-tidy/ClangTidyTest.h
188–189

Is returning an empty string OK?

It seems like a reasonable behavior, but I've not tried it myself.

This should let the assertion on the test do the job.

If an assertion fails with assertions disabled, the resulting UB is generally acceptable. Because these are unit tests, I think we can generally rely on the assertion being enabled if this code is being run.

196

Ah, I didn't see that this was a copy paste from below. I think those code paths could be combined if you hoisted the code out to the function level.

jdemeule updated this revision to Diff 136389.Feb 28 2018, 1:47 PM

Update after review.

alexfh requested changes to this revision.Mar 1 2018, 8:25 AM
alexfh added inline comments.
unittests/clang-tidy/ClangTidyTest.h
145

I wonder whether it's better to use lit for the tests that require formatting than to expand clang-tidy unit test utilities with runCheckAndFormatOnCode? What was the reason to use unit tests in this patch as opposed to lit tests?

This revision now requires changes to proceed.Mar 1 2018, 8:25 AM
jdemeule added inline comments.Mar 6 2018, 1:02 PM
unittests/clang-tidy/ClangTidyTest.h
145

Indeed, that a good question.
Personally, I found unit test easier to launch and debug than lit test and I notice that it is not possible to made a unit test which react the same way as clang-tidy.
When I wrote this test, I want it to react as in clang-tidy and for this I try to call cleanupAroundReplacements in the test.
Adding formatting was more a side effect to have some sustainable and comprehensive way to write test.

alexfh added inline comments.Mar 7 2018, 8:48 AM
unittests/clang-tidy/ClangTidyTest.h
145

With more features clang-tidy gets the discrepancies between how unit tests work and how clang-tidy works become larger. I wouldn't like to mirror every clang-tidy feature in the code that supports unit tests. We should either expose more high-level APIs or use clang-tidy itself and lit tests. (I personally like the latter more, since lit tests tend to be closer to the real use cases and are easier to write and iterate on.)

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

chgans added a subscriber: chgans.May 5 2019, 2:30 PM

Hi, Using llvm-8 (llvm ubuntu repo), clang-replacement has improved, but there still a case when a comma is left beyond: if there was only one member init.

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

Hi, now, this patch contains only tests which are already covered with lit tests.
The cleanup is now applied in clang-apply-replacements.

Hi, Using llvm-8 (llvm ubuntu repo), clang-replacement has improved, but there still a case when a comma is left beyond: if there was only one member init.

I do not reproduce at master (9.0.0 trunk), do you have an example in mind?

lebedev.ri added a subscriber: lebedev.ri.EditedJun 17 2019, 7:04 AM

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

Hi, now, this patch contains only tests which are already covered with lit tests.
The cleanup is now applied in clang-apply-replacements.

How does that work with clang-tidy -fix ?

Hi, Using llvm-8 (llvm ubuntu repo), clang-replacement has improved, but there still a case when a comma is left beyond: if there was only one member init.

I do not reproduce at master (9.0.0 trunk), do you have an example in mind?

What is the status here? Will you continue to work on this patch @jdemeule (which would be great!) ?

Hi, now, this patch contains only tests which are already covered with lit tests.
The cleanup is now applied in clang-apply-replacements.

How does that work with clang-tidy -fix ?

If I remember by calling 'format::cleanupAroundReplacements' when applying the fix in memory.