[PATCH] D26750: [clang-tidy] Add modernize-use-default-member-init check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:34
+  case Type::STK_IntegralComplex:
+    return InitType->isCharType() ? "'\\0'" : "0";
+  case Type::STK_Floating:
----------------
malcolm.parsons wrote:
> aaron.ballman wrote:
> > malcolm.parsons wrote:
> > > aaron.ballman wrote:
> > > > 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).
> > > `isCharType()` returns true for narrow character types only.
> > > So if this is incorrect, wouldn't your suggestion be incorrect too?
> > 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.
> 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.
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.


https://reviews.llvm.org/D26750