This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add modernize-use-default-member-init check
ClosedPublic

Authored by malcolm.parsons on Nov 16 2016, 8:35 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

malcolm.parsons retitled this revision from to [clang-tidy] Add modernize-use-default-member-init check.
malcolm.parsons updated this object.
malcolm.parsons added a subscriber: cfe-commits.
docs/clang-tidy/checks/modernize-use-default-member-init.rst
7 ↗(On Diff #78200)

Please fix double spaces. Same below.

Eugene.Zelenko edited reviewers, added: hokein; removed: Eugene.Zelenko.Nov 16 2016, 10:06 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added subscribers: Prazek, Eugene.Zelenko.
docs/clang-tidy/checks/modernize-use-default-member-init.rst
7 ↗(On Diff #78200)

If you like, but what harm are they causing?

aaron.ballman added inline comments.Nov 17 2016, 5:59 AM
docs/clang-tidy/checks/modernize-use-default-member-init.rst
7 ↗(On Diff #78200)

Consistency; double-spacing is atypical in our documentation, probably because it's generally viewed online rather than in print.

Remove doubled spaces

Use llvm_unreachable.
Ignore template instantiations.

Handle unary operators.

How does check handle bit fields? I encountered problem on next code in ASTContext.h:

struct TypeInfo {

uint64_t Width;
unsigned Align;
bool AlignIsRequired : 1;

TypeInfo() : Width(0), Align(0), AlignIsRequired(false) {}

};

Will be also good idea to add test case for bit fields.

How does check handle bit fields?

There is no warning for bit fields.
I don't think the C++17 bit field initializer proposal has been accepted yet.

Will be also good idea to add test case for bit fields.

There is a test:

struct NegativeBitField
{
  NegativeBitField() : i(6) {}
  int i : 5;
};

Use hasUnaryOperand.

alexfh added inline comments.Dec 13 2016, 8:15 AM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
21 ↗(On Diff #80869)

The function name doesn't make it clear that the value is returned for zero-initialization.

137 ↗(On Diff #80869)

isInTemplateInstantiation() is usually a better choice, since it also checks for ancestors being templates.

157–161 ↗(On Diff #80869)

This is a more common way to do this in LLVM:

if (const auto *Default = Result.Nodes.getNodeAs<...>("default"))
  checkDefaultInit(Result, Default);
else if (const auto *Existing = ...)
  checkExistingInit(...);
else
  llvm_unreachable(...);
docs/clang-tidy/checks/modernize-use-default-member-init.rst
29 ↗(On Diff #80869)

Is an empty line needed after this?

malcolm.parsons planned changes to this revision.Dec 13 2016, 8:50 AM
malcolm.parsons added inline comments.
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
21 ↗(On Diff #80869)

getValueOfValueInit?

Rename function.
Use isInstantiated().
Reduce variable scope.

docs/clang-tidy/checks/modernize-use-default-member-init.rst
29 ↗(On Diff #80869)

modernize-use-equals-default.rst doesn't have one.

aaron.ballman added inline comments.Dec 19 2016, 9:14 AM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
33–34 ↗(On Diff #81310)

Do these require a literal suffix to avoid conversion?

62 ↗(On Diff #81310)

Elide braces.

70 ↗(On Diff #81310)

Elide braces.

77–78 ↗(On Diff #81310)

Do you also want to ignore paren expressions?

80 ↗(On Diff #81310)

Elide braces (same elsewhere).

103 ↗(On Diff #81310)

Perhaps character and string literals as well?

106 ↗(On Diff #81310)

Hoist this into the default case and put an llvm_unreachable() instead?

119 ↗(On Diff #81310)

This would be useful as a public matcher, I think. Fine to do in a follow-up patch if you prefer.

123 ↗(On Diff #81310)

This check should not register matchers for C++ < 11.

docs/clang-tidy/checks/modernize-use-default-member-init.rst
6 ↗(On Diff #81310)

You should mention that the check does nothing in versions older than C++11.

30 ↗(On Diff #81310)

"enums, and pointers", because Oxford commas are the best commas. ;-)

malcolm.parsons marked 9 inline comments as done.Dec 19 2016, 12:07 PM
malcolm.parsons added inline comments.
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
77–78 ↗(On Diff #81310)

Maybe.
Am I reimplementing some existing clang function here?

123 ↗(On Diff #81310)

I always forget to add a condition here; maybe add_new_check.py could remind me.

aaron.ballman added inline comments.Dec 19 2016, 12:09 PM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
77–78 ↗(On Diff #81310)

You can use IgnoreParenImpCasts() instead.

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
77–78 ↗(On Diff #81310)

I mean is sameValue() implemented elsewhere?

aaron.ballman added inline comments.Dec 19 2016, 12:15 PM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
77–78 ↗(On Diff #81310)

Not in a general form in Clang, at least that I am aware of.

malcolm.parsons marked 10 inline comments as done.

Use IgnoreParenImpCasts().
Elide braces.
Handle character and string literals.
Require C++11.
Improve doc.

malcolm.parsons marked an inline comment as done.

Use 0 for IntegralComplex.

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
33–34 ↗(On Diff #81310)

I don't think so, but using 0.0 for an integral complex value doesn't look right.

aaron.ballman added inline comments.Dec 20 2016, 5:31 AM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
34 ↗(On Diff #82073)

This is incorrect if the char type is not a narrow character type. I would probably just initialize the integral with 0, regardless of whether it was a character or not. You should add a test for char, wchar_t, char16_t (et al), and probably all of the other types (just to make sure we handle them properly and don't introduce later regressions).

112 ↗(On Diff #82073)

You'll need to add the llvm_unreachable() here in order to avoid MSVC warnings about not all control paths returning a value.

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
34 ↗(On Diff #82073)

isCharType() returns true for narrow character types only.
So if this is incorrect, wouldn't your suggestion be incorrect too?

112 ↗(On Diff #82073)

AFAIK, that's when a switch covers all valid enum values but doesn't have a default case.
This switch has a default case.

aaron.ballman added inline comments.Dec 20 2016, 6:11 AM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
34 ↗(On Diff #82073)

Ah, I think I made a confusing statement. By "incorrect", I meant, "inconsistent." For int, you would get 0, for char, you would get '\0', but for wchar_t you would get 0 instead of L'\0', for char16_t you would get 0 instead of u'\0', etc. Rather than deal with all of the prefixing, I think it's better to just initialize with 0 for all integral types.

112 ↗(On Diff #82073)

Yes, but MSVC often doesn't pick up on that fact. We've used this pattern in the past (see getAbsoluteValueFunctionKind() in SemaChecking.cpp), but perhaps this is fine and we can deal with any diagnostics post-commit (I didn't check whether the warning really happens, I was going from memory).

clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
34 ↗(On Diff #82073)

All of these scalar types could be initialised with 0, but I prefer nullptr, false, '\0' and 0.0 for pointers, bool, char and float respectively.
If I used wchar_t, char16_t or char32_t, I'd probably want the prefixing.

aaron.ballman added inline comments.Dec 20 2016, 7:58 AM
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
34 ↗(On Diff #82073)

I can agree that this may be a matter of preference, but my point still stands. If we're going to use a literal type that matches the variable type, we should do that consistently. So if you want to go with '\0' for a char, please use L'\0' for a wchar_t, and similar for the other character types.

You may also want to consider using the f suffix for a float rather than relying on the narrowing conversion from double to float. Similar for the complex type suffixes.

Use character literal prefixes.
Use float literal suffix.

aaron.ballman accepted this revision.Dec 20 2016, 12:41 PM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Dec 20 2016, 12:41 PM
This revision was automatically updated to reflect the committed changes.