This is an archive of the discontinued LLVM Phabricator instance.

clang-tidy: improve the 'modernize-use-default-member-init'
ClosedPublic

Authored by oleg.smolsky on Dec 2 2021, 1:50 PM.

Details

Summary
  • we want to deal with non-default constructors that just happen to contain constant initializers

There was already a negative test case, it is now a positive one. We find and refactor this case:

struct PositiveNotDefaultInt {
  PositiveNotDefaultInt(int) : i(7) {}
  int i;
};

Diff Detail

Event Timeline

oleg.smolsky created this revision.Dec 2 2021, 1:50 PM
oleg.smolsky requested review of this revision.Dec 2 2021, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2021, 1:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fixing the uploaded diff...

Ran clang-format

Ran clang-format on the test cases.

@aaron.ballman do you happen to know who is best to review a clang-tidy fix?

Adding some additional reviewers. At a high level, I think this is a reasonable direction to go, but I wonder if this should be hidden behind a configuration option or not (basically, was there a reason we didn't cover that case originally or was it an oversight/left for future work)?

clang-tools-extra/test/clang-tidy/checkers/modernize-use-default-member-init.cpp
48–49

The "default" here is not about the constructor but about the argument to the constructor, I believe. So I think this should be renamed to PositiveConstantMemInit or something like that (similar for the names below).

Sure, adding an option is easy, if that's the consensus. What would you call it?

was there a reason we didn't cover that case originally or was it an oversight/left for future work?

It was left for future work - by only considering the initializer list of the default constructor, clang-tidy did not have to work out what to do when the constructors don't agree on what value the member init should have.
The next step towards the full solution is to handle classes with only one non-special constructor.

was there a reason we didn't cover that case originally or was it an oversight/left for future work?

It was left for future work - by only considering the initializer list of the default constructor, clang-tidy did not have to work out what to do when the constructors don't agree on what value the member init should have.

Thank you for verifying! @oleg.smolsky -- this would be a very useful test case to add, btw.

Sure, adding an option is easy, if that's the consensus. What would you call it?

Since we left this for future work, I don't think we need to add a configuration option unless a user finds a need for one.

Added a "two constructors" test case along with the support for that.

was there a reason we didn't cover that case originally or was it an oversight/left for future work?

It was left for future work - by only considering the initializer list of the default constructor, clang-tidy did not have to work out what to do when the constructors don't agree on what value the member init should have.

Thank you for verifying! @oleg.smolsky -- this would be a very useful test case to add, btw.

Yep, done.

PLAL

Mostly just minor nits at this point. Please be sure to update the documentation and add a release note about the changes.

clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
-2

Spurious newline?

250
251–252
oleg.smolsky marked 2 inline comments as done.

Addressed review comments: explicit types and doc changes.

Done with review changes.

clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
-2

yep, removing it...

This revision is now accepted and ready to land.Jan 3 2022, 10:32 AM

@aaron.ballman could you commit this change please? I've never had commit rights... Thanks!

@aaron.ballman could you commit this change please? I've never had commit rights... Thanks!

Sure can! Is Oleg Smolsky <oleg.smolsky@gmail.com> the correct attribution for the patch, or would you like me to use a different name or email address?

@aaron.ballman could you commit this change please? I've never had commit rights... Thanks!

Sure can! Is Oleg Smolsky <oleg.smolsky@gmail.com> the correct attribution for the patch, or would you like me to use a different name or email address?

Thanks Aaron! Yes, everything here looks right.

aaron.ballman closed this revision.Jan 4 2022, 4:28 AM

Thanks for the improvement! I've commit on your behalf in 051847cfecaea3f55fc4f822facfbf5d21bde8dd.