Page MenuHomePhabricator

[clang-tidy] New check cppcoreguidelines-prefer-member-initializer
Needs ReviewPublic

Authored by baloghadamsoftware on Dec 9 2019, 5:03 AM.

Details

Summary

Finds member initializations in the constructor body which can be placed into the initialization list instead. This does not only improves the readability of the code but also affects positively its performance. Class-member assignments inside a control statement or following the first control statement are ignored.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 5:03 AM

affects positively its performance.

Interesting! Do you have some numbers you could share? thanks

Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check.

clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp
19 ↗(On Diff #232816)

I'm working on a checker which has the need for similarly knowing occurrences of "control flow breaking statements". How about goto and calling a [[noreturn]] function, such as (std::)longjmp? Or there is no point in matching such in your checker?

71 ↗(On Diff #232816)

FIXME remained. Did you upload the right patch set?

85 ↗(On Diff #232816)

can -> should?

clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.h
18 ↗(On Diff #232816)

FIXME remained here.

clang-tools-extra/docs/clang-tidy/checks/readability-prefer-initialization-list.rst
7 ↗(On Diff #232816)

improves -> improve

8 ↗(On Diff #232816)

word order: also positively affects

27 ↗(On Diff #232816)

typo: construcotr

27–28 ↗(On Diff #232816)

[l]ist, unlike m, as m's initialization follow a control statement (if)

whisperity added inline comments.Dec 9 2019, 5:45 AM
clang-tools-extra/test/clang-tidy/checkers/readability-prefer-initialization-list.cpp
221 ↗(On Diff #232816)

Comment diverged from what's actually written in the code.

Clang-tidy also has modernize-use-default-member-init. Will be good idea to mention this check in documentation and otherwise as well as draw distinction (C++ version, coding guidelines, etc) in use cases.

clang-tools-extra/clang-tidy/readability/PreferInitializationListCheck.cpp
66 ↗(On Diff #232816)

Will be good idea to check for C++.

clang-tools-extra/docs/ReleaseNotes.rst
73

Please synchronize with first statement in documentation.

baloghadamsoftware retitled this revision from [clang-tidy] New check readability-prefer-initialization-list to [clang-tidy][WIP] New check readability-prefer-initialization-list.Dec 10 2019, 12:16 AM

Updated according to some comments.

baloghadamsoftware marked 7 inline comments as done.Dec 10 2019, 6:48 AM

Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check.

This worries me as well -- we already have checks that prefer doing an in-class initialization to using a constructor member initialization list. How should this check interact with modernize-use-default-member-init?

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
91 ↗(On Diff #233087)

I have a concern with the name using initialization-list -- that sounds a lot like the check is going to prefer {42} to = 42. Perhaps the name should be prefer-ctor-member-initialization or something along those lines?

Can you refresh my memory on whether a rule for "if init expr is constant, initialise in class body instead" exists for init list members? If so, this will be a funny "two pass needed to fix" kind of check.

This worries me as well -- we already have checks that prefer doing an in-class initialization to using a constructor member initialization list. How should this check interact with modernize-use-default-member-init?

This check is to enforce C++ core guideline C.49 while the modernize check enforces guideline C.48. The two must be synchronized, and I think that this new check should do that: for initializations that should be done using in-class initializers according to the other checker this checker must suggest the same. For the rest we should suggest member initializers in the constructor.

This check is to enforce C++ core guideline C.49 while the modernize check enforces guideline C.48. The two must be synchronized, and I think that this new check should do that: for initializations that should be done using in-class initializers according to the other checker this checker must suggest the same. For the rest we should suggest member initializers in the constructor.

It should be in cppcoreguidelines module.

baloghadamsoftware retitled this revision from [clang-tidy][WIP] New check readability-prefer-initialization-list to [clang-tidy][WIP] New check cppcoreguidelines-prefer-initialization-list.

Now checker proposes default member initialization if applicable. Thus it is in sync with checker modernize-use-default-member-init. Also moved to ícppcoreguidelinesí group.

baloghadamsoftware retitled this revision from [clang-tidy][WIP] New check cppcoreguidelines-prefer-initialization-list to [clang-tidy] New check cppcoreguidelines-prefer-initialization-list.Jan 15 2020, 1:26 AM
Eugene.Zelenko added inline comments.Jan 15 2020, 3:15 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
53

Please elide braces.

clang-tools-extra/docs/ReleaseNotes.rst
70

Is it relevant?

73

Is it relevant?

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
16

Enum should be in double back-ticks as language construct.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.

Updated according to the comments.

baloghadamsoftware marked 6 inline comments as done.Jan 15 2020, 4:17 AM

Thank you for the comments.

clang-tools-extra/docs/ReleaseNotes.rst
70

Absouletely not, it is nonsense. Just added by the renaming script without me knowing about it.

73

The same as above.

By the word, please rebase, because Release Notes were reset after version 10 branching.

baloghadamsoftware marked 2 inline comments as done.

Rebased.

Eugene.Zelenko added inline comments.Jan 16 2020, 6:03 AM
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-prefer-member-initializer.rst
73

Please make indentation 2 characters, same as in above code snippets.

baloghadamsoftware retitled this revision from [clang-tidy] New check cppcoreguidelines-prefer-initialization-list to [clang-tidy] New check cppcoreguidelines-prefer-initializer.

Indentation of the code fixed in the documentation.

baloghadamsoftware retitled this revision from [clang-tidy] New check cppcoreguidelines-prefer-initializer to [clang-tidy] New check cppcoreguidelines-prefer-member-initializer.Jan 16 2020, 9:31 AM
aaron.ballman added inline comments.Sat, Feb 1, 8:04 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
47

Please keep this list sorted alphabetically.

clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
43

What about other kinds of literals like user-defined literals, or literal class types? Should this be using E->getType()->isLiteralType()?