This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add check 'readability-redundant-member-init'
ClosedPublic

Authored by malcolm.parsons on Sep 8 2016, 5:52 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

malcolm.parsons retitled this revision from to [clang-tidy] Add check 'readability-redundant-member-init'.
malcolm.parsons updated this object.
Eugene.Zelenko set the repository for this revision to rL LLVM.Sep 8 2016, 10:43 AM

I think will be good idea to add cases when member is initialized in declaration and constructor, with same and different values.

docs/clang-tidy/checks/readability-redundant-member-init.rst
4 ↗(On Diff #70684)

This line length should be same as header above.

13 ↗(On Diff #70684)

Please run Clang-format over example. Will be good idea to add empty line before private.

test/clang-tidy/readability-redundant-member-init.cpp
33 ↗(On Diff #70684)

Please remove extra empty-line.

Prazek added inline comments.Sep 8 2016, 11:33 AM
clang-tidy/readability/RedundantMemberInitCheck.cpp
34 ↗(On Diff #70684)

Arguments (upper case)

Eugene.Zelenko added inline comments.Sep 8 2016, 11:34 AM
clang-tidy/readability/RedundantMemberInitCheck.cpp
36 ↗(On Diff #70684)

begin() and end() are not used extensively. Why not to use std::?

sbenza added inline comments.Sep 8 2016, 11:44 AM
clang-tidy/readability/RedundantMemberInitCheck.cpp
33 ↗(On Diff #70684)

These construct expressions might actually have side effects that you would not get if you omit them.
For example, aggregates will be value initialized. If you remove it you change the behavior.

The tests should include defaulted vs non-defaulted default constructor, user-defined/user-provided/implicit default constructor, aggregates with and without trivially constructible members, etc.

34 ↗(On Diff #70684)

Arguments variable name. (should start with upper case)

36 ↗(On Diff #70684)

There's no need for these using declarations.
Just do arguments.begin()/arguments.end().
They are using in generic code. This is not generic code.

39 ↗(On Diff #70684)

You want std::none_of instead of std::find_if.
(or use std::any_of and don't negate the expression in the lambda)

docs/clang-tidy/checks/readability-redundant-member-init.rst
6 ↗(On Diff #70684)

Explain when they are unnecessary.

aaron.ballman added inline comments.Sep 8 2016, 11:54 AM
clang-tidy/readability/RedundantMemberInitCheck.cpp
34 ↗(On Diff #70684)

Please do not use auto here, since the type is not spelled out in the initializer.

clang-tidy/readability/RedundantMemberInitCheck.h
19 ↗(On Diff #70684)

What makes one unnecessary? Also, missing a full stop at the end of the sentence.

How do I add FixIt hints?
They should be simple removals, but how do I decide whether to remove the following comma, preceding comma or preceding colon?

clang-tidy/readability/RedundantMemberInitCheck.cpp
33 ↗(On Diff #70684)

Yes, more tests needed.

39 ↗(On Diff #70684)

Maybe I only need to check the first argument - if that's defaulted then they all are.

docs/clang-tidy/checks/readability-redundant-member-init.rst
6 ↗(On Diff #70684)

"Finds member initializations that are unnecessary because the same default constructor would be called if they were not present"

Improve comments
Improve doc
Add tests
Simplify implementation

malcolm.parsons marked 15 inline comments as done.Sep 9 2016, 2:57 AM
malcolm.parsons added inline comments.
clang-tidy/readability/RedundantMemberInitCheck.cpp
34 ↗(On Diff #70797)

Other forms of initialization are not CXXConstructExprs.

alexfh edited edge metadata.Sep 14 2016, 5:44 AM

How do I add FixIt hints?
They should be simple removals, but how do I decide whether to remove the following comma, preceding comma or preceding colon?

Just remove the entry and leave the comma and the colon alone. Clang-tidy needs to learn to clean up the code after such replacements using clang-format's cleanupAroundReplacements. Filed https://llvm.org/bugs/show_bug.cgi?id=30379

malcolm.parsons edited edge metadata.

Handle unions and templated classes.
Add FixItHints (depends on D24572).

Handle delegating and base class constructors

Don't remove init of const members.
Do remove calls to constructors that introduce cleanups.

Don't warn for trivially default constructable members

aaron.ballman added inline comments.Oct 4 2016, 7:09 AM
test/clang-tidy/readability-redundant-member-init.cpp
162 ↗(On Diff #73260)

Missing a test for union member initializers. Also, a test that multiple inheritance doesn't cause a fixit malfunction would be nice.

malcolm.parsons added inline comments.Oct 4 2016, 7:42 AM
test/clang-tidy/readability-redundant-member-init.cpp
162 ↗(On Diff #73260)

Note that all the fixits malfunction until D24572 lands.

Add test for initializing a union member.
Add test for multiple inheritance.

aaron.ballman accepted this revision.Oct 10 2016, 6:17 AM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Oct 10 2016, 6:17 AM
malcolm.parsons edited edge metadata.

Use ignoringImplicit

This revision was automatically updated to reflect the committed changes.