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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to