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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
