Added the remaining features needed to satisfy C++ Core Guideline Type.6: Always initialize a member variable to cppcoreguidelines-pro-type-member-init. The check now flags all default-constructed uses of record types without user-provided default constructors that would leave their memory in an undefined state. The check suggests value initializing them instead.
Details
- Reviewers
flx aaron.ballman alexfh - Commits
- rG855d97e30cae: Complete support for C++ Core Guidelines Type.6: Always initialize a member…
rCTE266191: Complete support for C++ Core Guidelines Type.6: Always initialize a member…
rL266191: Complete support for C++ Core Guidelines Type.6: Always initialize a member…
Diff Detail
Event Timeline
Added explanation of changes to ReleaseNotes.rst and cleaned up documentation for the check a little.
Thank you for the patch! I'd like the original author to take a look at it, so just a couple of peanut gallery comments from me for now.
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp | ||
---|---|---|
53 | I wonder whether some relevant parts of SemaExprCXX.cpp could be made reusable so that you don't have to completely reimplement those. | |
58 | Please clang-format the code. |
Fixed tests after clang-format.
Fixed checks related to non-trivial compiler-generated default constructors that weren't accounted for in the previous iteration.
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp | ||
---|---|---|
25 | Please do not remove the unnamed namespace; it is preferable to using static functions. | |
32 | Missing a period at the end of the sentence. | |
35 | This comment doesn't match the behavior of the code below. | |
35–36 | Please use && instead of and. | |
109 | This should probably be made more generally available as an AST matcher. | |
109–126 | Please use || instead of or. Also, asserts should usually have && "Some explanatory text" for when the assert is triggered. | |
113 | I think this matcher logic should go into utils\TypeTraits.cpp as it seems more generally useful than just this check. | |
117 | As should this. | |
180 | Since this function is only used once, I think it should be lowered into its use as a ternary conditional. | |
193 | Please do not use an initializer list like this, I believe it won't work in MSVC 2013. (Here and elsewhere as well.) | |
195 | Please only use auto when the type name is spelled out as part of the initializer, or it is used as a for-range initializer. | |
257 | Can use const auto & here instead. | |
259–283 | Since this is a C++ core guideline, I think the matchers should only be registered when compiling for C++. It may make sense to use a similar check in C mode, but we can cross that bridge when we come to it (and have tests for it). | |
409 | I think this code belongs in fixInitializerList then, doesn't it? | |
420 | You should be able to pass in Var directly. | |
test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp | ||
285 | s/on/one | |
291 | s/on/one | |
334 | 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; }; |
Addressed comments.
Moved isTriviallyDefaultConstructible into utils/TypeTraits.
Moved AST matchers into utils/Matchers.h.
Enclosed all source-local functions in an anonymous namespace and removed static.
Replaced stray ands and ors with && and ||.
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!
Edit: Huh. Didn't realize those comments I had written didn't get posted. I'm new to Phabricator...
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp | ||
---|---|---|
25 | 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: | |
53 | 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. | |
180 | 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. | |
409 | 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. | |
test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp | ||
334 | For a number of reasons, it doesn't hit any warnings:
|
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp | ||
---|---|---|
157 | getCanonicalRecordDecl() instead? (Otherwise, this suggests the function could be replaced with Type->getCXXRecordDecl() at the call site, but that's not quite right because this function canonicalizes the type first.) | |
clang-tidy/utils/Matchers.h | ||
31 | This should go into clang\include\clang\ASTMatchers\ASTMatchers.h instead. When added there, it should get doxygen comments, appropriate test cases in clang\unittests\ASTMatchers\ASTMatchersTest.cpp, and then you should regenerate the online documentation by running clang\docs\tools\dump_ast_matchers.py. | |
35 | Same for this one. |
Moved isDelegatingConstructor and isUserProvided into clang/include/clang/ASTMatchers/ASTMatchers.h and updated the tests and docs.
Renamed getRecordDecl to getCanonicalRecordDecl to better match its behavior.
LGTM with one minor correction (you can correct when committing it).
unittests/ASTMatchers/ASTMatchersTest.cpp | ||
---|---|---|
2343 | I think this test got duplicated accidentally; I don't see it removed as part of the diff. |
I can commit the patch for you, but you need to split it in two: one for the cfe repository, one for clang-tools-extra.
Great—thanks! I have them separated already and had to merge them for the Phabricator review. Does that mean I should open another review for the cfe one with you as a reviewer and make this one only clang-tools-extra changes or is there an easier way for me to give the patches to you?
Yes, please create another Differential revision for the cfe patch. That's probably the easiest way to transfer patches ;)
FYI, the check has started crashing after this patch. I'll try to provide a minimal test case soon. The relevant part of the stack trace is:
@ 0x7fc9c255efa0 8 clang::Stmt::getLocStart() @ 0x7fc9c48fdac1 64 clang::tidy::cppcoreguidelines::(anonymous namespace)::IntializerInsertion::getLocation() @ 0x7fc9c49026d5 1696 clang::tidy::cppcoreguidelines::ProTypeMemberInitCheck::checkMissingBaseClassInitializer() @ 0x7fc9c490424f 96 clang::tidy::cppcoreguidelines::ProTypeMemberInitCheck::check()
Hmm... I can get this to crash in that location but only if I run with -fdelayed-template-parsing. I've got an easy fix and test case if that's the same issue you're running into. We should move the check at line 307 into ProTypeMemberInitCheck::check before both checks. I've got a test case I can add to cppcoreguidelines-pro-type-member-init-delayed.cpp that reproduces it. Let me know if I should write up a patch for this.
Would be nice, if you could write a patch. I don't yet have a reduced test
case (it crashes without delayed template parsing), since my creduce is
still running after many hours ;)
I have created a minimal test case, see https://llvm.org/bugs/show_bug.cgi?id=27419. I think it's related to template. It would be very nice if you can submit a patch fixing this (and include the crash test case).
Thanks for the test case! The same fix I had applies. I reduced your test case down even further and added it to the tests. I submitted a patch with these fixes (D19270).
This had been failing. Fixed in r267290.
See also http://bb.pgr.jp/builders/msbuild-llvmclang-x64-msc19-DA/builds/349
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp | ||
---|---|---|
120 ↗ | (On Diff #53537) | raw_string_ostream cannot be assumed as unbuffered, nor would be flushed by the destructor before Code copied to return value. |
Please do not remove the unnamed namespace; it is preferable to using static functions.