Page MenuHomePhabricator

[analyzer] UndefinedAssignment: Fix warning message on implicit copy/move constructors.
ClosedPublic

Authored by NoQ on Feb 26 2018, 5:46 PM.

Details

Summary

When a class forgets to initialize a field in the constructor, and then gets copied around, a warning is emitted that the value assigned to a specific field is undefined. When the copy/move constructor is implicit (not written out in the code) but not trivial (is not a trivial memory copy, eg. because members have an explicit copy constructor), the body of such constructor is auto-generated in the AST. In this case the checker's warning message is squeezed at the top of the class declaration, and it gets hard to guess which field is at fault.

Fix the warning message to include the name of the field.

Such warnings have become fairly popular in our temporary constructor evaluation.

This warning should probably be extended to the trivial copy case, but that's a separate story.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 26 2018, 5:46 PM
a.sidorin added inline comments.Feb 27 2018, 12:31 AM
lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
60 ↗(On Diff #136014)

SmallString<128>?

98 ↗(On Diff #136014)

May be it is better to print qualified name? Just name of member can be misleading if base classes have members with same names.

NoQ updated this revision to Diff 136145.Feb 27 2018, 1:30 PM
NoQ marked 2 inline comments as done.

Use SmallString.

Add some test for base-class implicit constructors to see how it currently works (not ideal but good enough).

lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
60 ↗(On Diff #136014)

*recalls how it's done* Ok!

98 ↗(On Diff #136014)

I don't think this can happen in an implicit constructor. We'd jump to the base-class constructor in this case.

dcoughlin accepted this revision.Feb 27 2018, 1:50 PM

LGTM. I have a suggestion for a slight modification of the diagnostic text inline.

test/Analysis/implicit-ctor-undef-value.cpp
12 ↗(On Diff #136145)

I think it would be good to mention that this assignment is happening in an implicit constructor, since that is probably not obvious to most users.

How about: "Value assigned to field 'y' in implicit constructor is garbage or undefined".

This revision is now accepted and ready to land.Feb 27 2018, 1:50 PM
NoQ updated this revision to Diff 136151.Feb 27 2018, 2:01 PM
NoQ marked an inline comment as done.

Mention the implicit constructor in the warning message.

This revision was automatically updated to reflect the committed changes.