Page MenuHomePhabricator

Complete support for C++ Core Guidelines Type.6: Always initialize a member variable.
ClosedPublic

Authored by michael_miller on Mar 29 2016, 4:13 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Can you summarize the improvements in docs/ReleaseNotes.rst, please?

Added explanation of changes to ReleaseNotes.rst and cleaned up documentation for the check a little.

alexfh edited edge metadata.Mar 31 2016, 5:17 AM

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
51 ↗(On Diff #51999)

I wonder whether some relevant parts of SemaExprCXX.cpp could be made reusable so that you don't have to completely reimplement those.

56 ↗(On Diff #51999)

Please clang-format the code.

michael_miller edited edge metadata.

Ran clang-format on all modified source files.

michael_miller marked an inline comment as done.

Fixed tests after clang-format.
Fixed checks related to non-trivial compiler-generated default constructors that weren't accounted for in the previous iteration.

aaron.ballman requested changes to this revision.Apr 5 2016, 9:29 AM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
25 ↗(On Diff #52272)

Please do not remove the unnamed namespace; it is preferable to using static functions.

30 ↗(On Diff #52272)

Missing a period at the end of the sentence.

33 ↗(On Diff #52272)

This comment doesn't match the behavior of the code below.

104 ↗(On Diff #52272)

Please use && instead of and.

152 ↗(On Diff #52272)

This should probably be made more generally available as an AST matcher.

156 ↗(On Diff #52272)

I think this matcher logic should go into utils\TypeTraits.cpp as it seems more generally useful than just this check.

160 ↗(On Diff #52272)

As should this.

187 ↗(On Diff #52272)

Please use || instead of or. Also, asserts should usually have && "Some explanatory text" for when the assert is triggered.

243 ↗(On Diff #52272)

Since this function is only used once, I think it should be lowered into its use as a ternary conditional.

256 ↗(On Diff #52272)

Please do not use an initializer list like this, I believe it won't work in MSVC 2013. (Here and elsewhere as well.)

258 ↗(On Diff #52272)

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.

296 ↗(On Diff #52272)

Can use const auto & here instead.

323 ↗(On Diff #52272)

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).

486 ↗(On Diff #52272)

I think this code belongs in fixInitializerList then, doesn't it?

497 ↗(On Diff #52272)

You should be able to pass in Var directly.

test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
285 ↗(On Diff #52272)

s/on/one

291 ↗(On Diff #52272)

s/on/one

334 ↗(On Diff #52272)

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;
};
This revision now requires changes to proceed.Apr 5 2016, 9:29 AM
michael_miller edited edge metadata.

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 ||.

A couple of nits.

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
181 ↗(On Diff #52752)

If you're doing this to disambiguate the ternary operator, you can skip the second cast.

test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
285 ↗(On Diff #52752)

nit: Trailing period is missing.

Same below.

alexfh requested changes to this revision.Apr 7 2016, 11:24 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 7 2016, 11:24 AM
michael_miller edited edge metadata.
michael_miller marked 15 inline comments as done.

Removed a superfluous cast and added periods to some comments.

alexfh accepted this revision.Apr 8 2016, 1:33 AM
alexfh edited edge metadata.

Looks good from my side. Please wait for the Aaron's approval.

Thank you for working on this!

michael_miller marked 4 inline comments as done.EditedApr 8 2016, 1:58 AM

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 ↗(On Diff #52272)

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

51 ↗(On Diff #52272)

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.

243 ↗(On Diff #52272)

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.

486 ↗(On Diff #52272)

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 ↗(On Diff #52272)

For a number of reasons, it doesn't hit any warnings:

  1. We don't do anything to record types without any members.
  2. U doesn't have a user-provided constructor. We only initialize members if there's a user-provided constructor of some sort.
  3. 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.
aaron.ballman added inline comments.Apr 11 2016, 7:11 AM
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
157 ↗(On Diff #52950)

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 ↗(On Diff #52950)

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 ↗(On Diff #52950)

Same for this one.

michael_miller edited edge metadata.
michael_miller marked 2 inline comments as done.

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.

michael_miller marked 3 inline comments as done.Apr 12 2016, 4:43 AM

Sounds good! Updated with suggested changes.

aaron.ballman accepted this revision.Apr 12 2016, 6:04 AM
aaron.ballman edited edge metadata.

LGTM with one minor correction (you can correct when committing it).

unittests/ASTMatchers/ASTMatchersTest.cpp
2343 ↗(On Diff #53380)

I think this test got duplicated accidentally; I don't see it removed as part of the diff.

This revision is now accepted and ready to land.Apr 12 2016, 6:04 AM
michael_miller edited edge metadata.

Removed accidentally duplicated test case.

LGTM with one minor correction (you can correct when committing it).

Oops! Not sure how that happened. I submitted a revised diff.

michael_miller marked an inline comment as done.Apr 12 2016, 1:10 PM

LGTM with one minor correction (you can correct when committing it).

Also, what's the next step? Is there anything else for me to do?

Do you have svn commit access?

Do you have svn commit access?

I do not... this is my first contribution to llvm/clang.

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.

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?

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 ;)

Broke out the cfe parts into its own change (D19038).

This revision was automatically updated to reflect the committed changes.

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()

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.

alexfh added a subscriber: alexfh.Apr 18 2016, 6:11 PM

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 ;)

hokein added a subscriber: hokein.Apr 19 2016, 1:33 AM

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.

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).

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.

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

raw_string_ostream cannot be assumed as unbuffered, nor would be flushed by the destructor before Code copied to return value.