[PATCH] D18584: Complete support for C++ Core Guidelines Type.6: Always initialize a member variable.

[Michael Miller via cfe-commits]

michael_miller marked 4 inline comments as done.
michael_miller added a comment.

In http://reviews.llvm.org/D18584#395208, @alexfh wrote:

> Looks good from my side. Please wait for the Aaron's approval.
>
> Thank you for working on this!


No problem! Thanks for taking the time to review and give feedback. clang-tidy is such a great tool. It's my honor and pleasure to be able to contribute and give back even a little!


================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:25
@@ -24,3 +24,3 @@
 
-namespace {
 
----------------
aaron.ballman wrote:
> Please do not remove the unnamed namespace; it is preferable to using static functions.
Ok, I can put the anonymous namespace around everything and remove the static specifiers. I moved it down lower to enclose the local class definition because it wasn't previously. I would normally wrap the whole thing but the LLVM coding standards seem to say the opposite of what I'm used to doing with regard to static functions vs anonymous namespaces:
http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:51
@@ +50,3 @@
+  return true;
+}
+
----------------
I was wondering about that too but didn't really know how to proceed. I did come across SemaExprCXX.cpp while looking to see how std::is_trivially_default_constructible was implemented. Being unfamiliar with that part of the codebase, though, I didn't dig too far into how to either share that code or hook into it.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:243
@@ +242,3 @@
+// initializer.
+static const NamedDecl *getInitializerDecl(const CXXCtorInitializer *Init) {
+  if (Init->isMemberInitializer())
----------------
aaron.ballman wrote:
> Since this function is only used once, I think it should be lowered into its use as a ternary conditional.
I've gone ahead and done this but I think it's less readable afterwards. The reason it's problematic is because the return types from the two subexpressions aren't the same so it can't be used as a ternary conditional without a cast.

================
Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:486
@@ +485,3 @@
+
+  // Do not propose fixes in macros since we cannot place them correctly.
+  if (Ctor->getLocStart().isMacroID())
----------------
aaron.ballman wrote:
> I think this code belongs in `fixInitializerList` then, doesn't it?
Sure. I think one reason I didn't was because of line 426 which is structured similarly. Stylistically, I slightly prefer to test the condition explicitly rather than have an early return but it's perfectly fine either way.

================
Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:334
@@ +333,2 @@
+  // CHECK-FIXES: int X{};
+};
----------------
aaron.ballman wrote:
> I'd like to see how we handle the following test:
> ```
> struct S {
>   S(int i);
> };
> struct T {
>   T(float f);
> };
> 
> union U {
>   S s;
>   T t;
> };
> ```
For a number of reasons, it doesn't hit any warnings:
  # We don't do anything to record types without any members.
  # U doesn't have a user-provided constructor. We only initialize members if there's a user-provided constructor of some sort.
  # If we declared an instance of U as a local variable, we would have to explicitly initialize it. The default constructor is implicitly deleted because of S and T.








http://reviews.llvm.org/D18584