Fixes PR18858
Details
Diff Detail
- Build Status
Buildable 1445 Build 1445: arc lint + arc unit
Event Timeline
docs/clang-tidy/checks/modernize-use-default-member-init.rst | ||
---|---|---|
8 | Please fix double spaces. Same below. |
docs/clang-tidy/checks/modernize-use-default-member-init.rst | ||
---|---|---|
8 | If you like, but what harm are they causing? |
docs/clang-tidy/checks/modernize-use-default-member-init.rst | ||
---|---|---|
8 | Consistency; double-spacing is atypical in our documentation, probably because it's generally viewed online rather than in print. |
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.
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; };
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
22 | The function name doesn't make it clear that the value is returned for zero-initialization. | |
138 | isInTemplateInstantiation() is usually a better choice, since it also checks for ancestors being templates. | |
158–162 | 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 | ||
30 | Is an empty line needed after this? |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
22 | getValueOfValueInit? |
Rename function.
Use isInstantiated().
Reduce variable scope.
docs/clang-tidy/checks/modernize-use-default-member-init.rst | ||
---|---|---|
30 | modernize-use-equals-default.rst doesn't have one. |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
34–35 | Do these require a literal suffix to avoid conversion? | |
63 | Elide braces. | |
71 | Elide braces. | |
78–79 | Do you also want to ignore paren expressions? | |
81 | Elide braces (same elsewhere). | |
104 | Perhaps character and string literals as well? | |
107 | Hoist this into the default case and put an llvm_unreachable() instead? | |
120 | This would be useful as a public matcher, I think. Fine to do in a follow-up patch if you prefer. | |
124 | This check should not register matchers for C++ < 11. | |
docs/clang-tidy/checks/modernize-use-default-member-init.rst | ||
7 | You should mention that the check does nothing in versions older than C++11. | |
31 | "enums, and pointers", because Oxford commas are the best commas. ;-) |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
78–79 | You can use IgnoreParenImpCasts() instead. |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
78–79 | I mean is sameValue() implemented elsewhere? |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
78–79 | Not in a general form in Clang, at least that I am aware of. |
Use IgnoreParenImpCasts().
Elide braces.
Handle character and string literals.
Require C++11.
Improve doc.
Use 0 for IntegralComplex.
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
34–35 | I don't think so, but using 0.0 for an integral complex value doesn't look right. |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
35 | 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). | |
113 | 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 | ||
---|---|---|
35 | isCharType() returns true for narrow character types only. | |
113 | AFAIK, that's when a switch covers all valid enum values but doesn't have a default case. |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
35 | 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. | |
113 | 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 | ||
---|---|---|
35 | 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. |
clang-tidy/modernize/UseDefaultMemberInitCheck.cpp | ||
---|---|---|
35 | 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. |
The function name doesn't make it clear that the value is returned for zero-initialization.