Page MenuHomePhabricator

misc-uninitialized-field
AbandonedPublic

Authored by jbcoe on Jun 18 2015, 3:40 PM.

Details

Reviewers
alexfh
rsmith
Summary

A clang-tidy check to find uninitialized fields in C++ classes and structs.

Diff Detail

Event Timeline

jbcoe updated this revision to Diff 27968.Jun 18 2015, 3:40 PM
jbcoe retitled this revision from to misc-uninitialized-field.
jbcoe updated this object.
jbcoe edited the test plan for this revision. (Show Details)
jbcoe added reviewers: rsmith, alexfh, klimek.
jbcoe set the repository for this revision to rL LLVM.
jbcoe changed the visibility from "Public (No Login Required)" to "All Users".
jbcoe added a subscriber: Unknown Object (MLST).

Minor coding style nits.

clang-tidy/misc/UninitializedFieldCheck.cpp
32

No need for curly brackets.

35

No need for curly brackets.

63

No need for curly bracket.

69

No need for curly bracket.

73

No need for curly bracket.

80

No need for curly bracket.

94

80-column?

103

No need for curly brackets.

clang-tidy/misc/UninitializedFieldCheck.h
9

Perhaps this TODO should be addressed prior to the commit.

alexfh edited edge metadata.Jun 19 2015, 7:33 AM

The check would be interesting to some projects, but it's not generally accepted that all fields of structs/ classes need to be initialized in the constructor. They need to be initialized _before they are used_. Thus, it needs to be moved out of the "misc" module which is reserved for more generic checks that are fine for most projects (at least, for LLVM and for projects using Google C++ style).

Also, are you aware of any styles that require initialization of all class fields in the constructor? Would be nice to refer to them in the check description.

jbcoe added a comment.Jun 20 2015, 7:04 AM

I definitely agree that this check is not necessary so should not be on by default. I've worked in a few places where the house style required members to be initialised in constructors. I'm afraid the style-guides were not public though.

I can move this from 'misc' to 'utils' if that makes sense.

jbcoe added a comment.EditedJun 20 2015, 7:20 AM

The add_new_check.py script won't let me add to 'utils'. I'm not sure where my check should go. Perhaps another category could be added such as 'extra' or 'optional'?

jbcoe updated this revision to Diff 28078.Jun 20 2015, 7:48 AM
jbcoe edited edge metadata.
jbcoe removed rL LLVM as the repository for this revision.
jbcoe changed the visibility from "All Users" to "Public (No Login Required)".

Removed unnecessary {}.
Added doxygen comment for class.
Removed static function used to register single matcher.

jbcoe accepted this revision.Jun 20 2015, 7:54 AM
jbcoe added a reviewer: jbcoe.
This revision is now accepted and ready to land.Jun 20 2015, 7:54 AM
alexfh added inline comments.Jun 21 2015, 1:44 AM
clang-tidy/misc/UninitializedFieldCheck.cpp
15

#includes should be sorted (e.g. using the llvm-include-order check, clang-tidy -fix should do the trick).

25

Variable names should start with a capital letter.

26
return f->getType()->isPointerType() || f->getType()->isBuiltinType();
42

nit: Please add trailing period

57

What about fields initialized in the constructor body?

71

In LLVM code, llvm::raw_string_ostream or llvm::raw_svector_ostream are used instead of std::stringstream.

83
95

We use the same style for messages, as Clang: first letter is lower-case, no trailing period.

jbcoe updated this revision to Diff 28092.Jun 21 2015, 1:31 PM
jbcoe removed a reviewer: jbcoe.

Style-related comments from review addressed.

Fixit text is now an insertion not a replacement.

Once people are happy I would like to move this check into a new category (optional?).

This revision now requires review to proceed.Jun 21 2015, 1:31 PM
alexfh added inline comments.Jun 22 2015, 3:26 AM
clang-tidy/misc/UninitializedFieldCheck.cpp
26

Did you miss this comment?

57

What about this?

71

Did you miss this comment?

jbcoe added inline comments.Jun 22 2015, 12:28 PM
clang-tidy/misc/UninitializedFieldCheck.cpp
15

I've applied my understanding of http://llvm.org/docs/CodingStandards.html#include-style. The clang-tidy check you suggested running made no changes to the code.

Please let me know if I've misunderstood anything.

25

I'll change this.
Out of curiosity, why is this LLVM's way of naming variables? I've not seen this style used elsewhere.

26

yes. I will address this.

57

My check does not take that into account. I think it's hard to do in general so I decided to let the user decide to ignore the check and proposed change. I presume that the optimiser is smart enough to do the right thing if a variable is obviously initialized in the constructor body and constructor field initializers.

57

I got confused by comment submission. Comment added.

71

I'm using a string instead. llvm::raw_string_ostream takes a string ref as a constuctor argument and the code is simpler if I just use the string directly.

jbcoe updated this revision to Diff 28148.Jun 22 2015, 12:31 PM

Combined pointer and builtin checks into a single condition.

alexfh added inline comments.Jun 23 2015, 7:27 AM
clang-tidy/misc/UninitializedFieldCheck.cpp
15

The comment related to the version where unordered_set, sstream and iterator were included in this specific order. Now that the last two are gone, there's no issue.

25

I don't know if there was any reason to choose this specific style. Maybe ask Chris Lattner? ;)

57

This decision needs to be documented in the class comment. And it may make sense to look at the number of false positives resulting from specifically this case and estimate how many of those could be avoided by just looking at which fields are assigned inside the constructor body.

71

llvm::raw_string_ostream is preferred for efficiency reasons. Your current version creates temporary strings on each iteration. You could prevent this by appending the two strings separately or by using raw_string_stream. The latter seems like a more elegant solution to me.

73

nit: Capitalization. Trailing period.

clang-tidy/misc/UninitializedFieldCheck.h
19

80 columns

jbcoe added inline comments.Jun 23 2015, 11:02 AM
clang-tidy/misc/UninitializedFieldCheck.cpp
57

I'll look into this.

71

agreed.

jbcoe updated this revision to Diff 28265.Jun 23 2015, 11:04 AM

Addressed review comments.

Once this is in a decent state I'd like to move it to another check category. It does not belong in 'misc' but I do not want to confuse detailed review by moving it from one location to another. I propose adding an 'optional' checks category for checks which may or may not be desirable depending on in-house style.

klimek resigned from this revision.Jul 3 2015, 6:49 AM
klimek removed a reviewer: klimek.
mcrosier removed a subscriber: mcrosier.Jul 6 2015, 11:55 AM
jbcoe added a comment.Jul 11 2015, 2:26 PM

This work is stalled but not abandoned. I plan to resume work mid-August.

mgehre added a subscriber: mgehre.Oct 14 2015, 4:15 PM

This check is a close fit for
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type6-always-initialize-a-member-variable
The "enforcement" there says:

  • Issue a diagnostic for any constructor of a non-trivially-constructible type that does not initialize all member variables. To fix: Write a data member initializer, or mention it in the member initializer list.
  • Issue a diagnostic when constructing an object of a trivially constructible type without () or {} to initialize its members. To fix: Add () or {}.

We could then move this check to cppcoreguidelines module.

alexfh added a comment.Nov 5 2015, 2:04 PM

This check is a close fit for
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type6-always-initialize-a-member-variable
The "enforcement" there says:

  • Issue a diagnostic for any constructor of a non-trivially-constructible type that does not initialize all member variables. To fix: Write a data member initializer, or mention it in the member initializer list.
  • Issue a diagnostic when constructing an object of a trivially constructible type without () or {} to initialize its members. To fix: Add () or {}.

    We could then move this check to cppcoreguidelines module.

Sounds like a great idea. Jonathan, do you still plan to work on this patch?

flx added a subscriber: flx.Dec 30 2015, 12:25 PM

What's the status of this patch? I'd be interested in picking it up if it is not being worked on.

jbcoe added a comment.Jan 7 2016, 2:59 PM

This patch is stalled. I've got distracted by other things.

@flx Please pick it up and see where you can take it.

jbcoe added a comment.Jan 7 2016, 3:02 PM

@alexfh apologies, I missed the email from your comment on Nov 5 2015.

jbcoe abandoned this revision.Jan 29 2016, 4:28 AM