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: > > 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. ================ Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:112 + } +} + ---------------- malcolm.parsons wrote: > aaron.ballman wrote: > > You'll need to add the `llvm_unreachable()` here in order to avoid MSVC > > warnings about not all control paths returning a value. > AFAIK, that's when a switch covers all valid enum values but doesn't have a > default case. > This switch has a default case. 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). https://reviews.llvm.org/D26750 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
