This is an archive of the discontinued LLVM Phabricator instance.

ClangTidy check to flag uninitialized builtin and pointer fields.
ClosedPublic

Authored by flx on Jan 23 2016, 9:54 PM.

Details

Summary

This patch is a continuation of http://reviews.llvm.org/D10553 by Jonathan B Coe.

The main additions are:

  1. For C++11 the check suggests in-class field initialization as fix. This makes the fields future proof towards the addition of new constructors.
  2. For older language versions the fields are added in the right position in the initializer list with more tests.
  3. User documentation.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 45817.Jan 23 2016, 9:54 PM
flx retitled this revision from to ClangTidy check to flag uninitialized builtin and pointer fields..
flx updated this object.
flx added reviewers: alexfh, jbcoe.
jbcoe edited edge metadata.Jan 24 2016, 5:04 AM

This looks good to me.

Fields initialised in constructors by being passed to methods by reference may be reported as false positives.

ie

void init(int& i);

struct A {
  int x;
  A() {
    init(x);
  }
};

I think it's going to be impossible to totally resolve this though as the body of initialisation functions may not be visible in the TU. False positives are better than silence in such cases.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: cfe-commits.
flx updated this revision to Diff 45837.Jan 24 2016, 5:14 PM
flx removed rL LLVM as the repository for this revision.

Removed unused include and changed casing on Builder variable.

flx updated this revision to Diff 45838.Jan 24 2016, 5:20 PM
alexfh edited edge metadata.Jan 26 2016, 8:41 AM

A high-level comment: I think, this comment still applies. I'm also slightly concerned about having this check in misc-, since the check isn't universally applicable (e.g. based on a couple of discussions, I guess LLVM community would likely not welcome this rule), but I'd like to leave misc- enabled by default. So moving the check to cppcoreguidelines- makes sense to me.

More substantial comments later.

flx added a comment.Jan 26 2016, 8:48 AM

A high-level comment: I think, this comment still applies. I'm also slightly concerned about having this check in misc-, since the check isn't universally applicable (e.g. based on a couple of discussions, I guess LLVM community would likely not welcome this rule), but I'd like to leave misc- enabled by default. So moving the check to cppcoreguidelines- makes sense to me.

More substantial comments later.

Sounds good to me. If I'm reading the original comment correctly what's missing would be this part:

"Issue a diagnostic when constructing an object of a trivially constructible type without () or {} to initialize its members. To fix: Add () or {}. "

Is that correct?

Please let me know if that changes the name of the check and whether the next step should be moving the check or we'll do that after a detailed review of the specifics.

Thanks,
Felix

flx added a comment.Feb 4 2016, 7:06 AM

Hi Alex,

could you take a look at the questions I posted in my last comment. Maybe there is some renaming work I can tackle while you review the change in more detail.

alexfh added a comment.Feb 5 2016, 5:28 AM
In D16517#336157, @flx wrote:

A high-level comment: I think, this comment still applies. I'm also slightly concerned about having this check in misc-, since the check isn't universally applicable (e.g. based on a couple of discussions, I guess LLVM community would likely not welcome this rule), but I'd like to leave misc- enabled by default. So moving the check to cppcoreguidelines- makes sense to me.

More substantial comments later.

Sounds good to me. If I'm reading the original comment correctly what's missing would be this part:

"Issue a diagnostic when constructing an object of a trivially constructible type without () or {} to initialize its members. To fix: Add () or {}. "

Yes. I think, we might want to either add a separate check for this or make this part configurable. Adding a FIXME for now would be enough.

Please let me know if that changes the name of the check and whether the next step should be moving the check or we'll do that after a detailed review of the specifics.

I'd call the check cppcoreguidelines-pro-type-memberinit to be consistent with the anchor name of this rule used in the CppCoreGuidelines document (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-memberinit). These anchor names are usually short enough, but still quite descriptive, so it makes sense to use them when there are no significantly better alternatives.

clang-tidy/misc/UninitializedFieldCheck.cpp
29 ↗(On Diff #45838)

Once you switch to SmallPtrSet, this should be changed to SmallPtrSetImpl&.

104 ↗(On Diff #45838)

That's a self-assignment, basically. Just use Stream.flush() or put the Stream definition into a nested scope.

106 ↗(On Diff #45838)

nit: No braces around single-line if/for bodies.

123 ↗(On Diff #45838)

SmallVector?

124 ↗(On Diff #45838)

.emplace_back(...)?

163 ↗(On Diff #45838)

I'd make a local matcher for this and push this check to the matcher.

169 ↗(On Diff #45838)

This should probably use SmallPtrSet.

170 ↗(On Diff #45838)

Consider this as a simpler alternative with fewer dependencies:

for (const auto &Field : ParentClass->fields()) {
  if (fieldRequiresInit(Field))
    FieldsToInit.insert(Field);
}

Also, might be reasonable to expand the fieldRequiresInit function:

for (const auto &Field : ParentClass->fields()) {
  if (Field->getType()->isPointerType() || Field->getType()->isBuiltinType())
    FieldsToInit.insert(Field);
}
177 ↗(On Diff #45838)

I think, this is superfluous. isMemberInitializer() should be enough.

181 ↗(On Diff #45838)

The variable does not make code cleaner, please remove it.

199 ↗(On Diff #45838)

nit: We don't use braces only for single-line if/for bodies. Same below.

207 ↗(On Diff #45838)

s/Builder/Diag/, which is more common in this code.

208 ↗(On Diff #45838)

No need to duplicate this code. Only the insertion of fixes should be conditional.

test/clang-tidy/misc-uninitialized-field.cpp
1 ↗(On Diff #45838)

As usual: tests with templates and macros, please ;)

6 ↗(On Diff #45838)

Also check that the constructor is left intact?

flx updated this revision to Diff 47159.Feb 7 2016, 8:43 PM
flx edited edge metadata.
flx marked 12 inline comments as done.

Thanks for the review, Alex!

This is an intermediate updated patch that addresses your detailed comments. I'll rename the whole check in the next couple of days and will re-upload again.

alexfh added a comment.Feb 8 2016, 5:30 AM

Looks good apart from renaming and one comment that's still relevant.

clang-tidy/misc/UninitializedFieldCheck.cpp
178 ↗(On Diff #47159)

What about this? (relates to line 184 now)

flx added inline comments.Feb 8 2016, 5:46 PM
clang-tidy/misc/UninitializedFieldCheck.cpp
178 ↗(On Diff #47159)

My comment on this was not submitted, just in case. Here it is again:

I think he idea is to return immediately if we're delegating to another constructor here. Am I misunderstanding the API call?

The important part is that we return in that case.

124 ↗(On Diff #45838)

Gave a compile error. Probably because there is no explicit constructor with the 3 arguments.

177 ↗(On Diff #45838)

I think he idea is to return immediately if we're delegating to another constructor here. Is this covered through some other check?

199 ↗(On Diff #45838)

Done. This seems really subtle though when it depends on line length instead of single statement inside of loop.

test/clang-tidy/misc-uninitialized-field.cpp
1 ↗(On Diff #45838)

Thanks for the reminder. Added early return for code inside of macros. Added template test which revealed I had to suppress the check inside of instantiated constructors.

flx updated this revision to Diff 47280.Feb 8 2016, 5:48 PM

Renamed the check to: cppcoreguidelines-pro-type-member-init, note the extra dash between member and init to be consistent with the camelcase notation of the class name ProTypeMemberInitCheck.

alexfh added inline comments.Feb 9 2016, 4:44 PM
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
88 ↗(On Diff #47280)

Maybe Lexer::getLocForEndOfToken?

94 ↗(On Diff #47280)

Maybe Lexer::getLocForEndOfToken?

175 ↗(On Diff #47280)

We still can output a warning, just be careful with fixits. Please also add a test for macros to illustrate the behavior. Two cases are interesting: code in a macro body and code in a macro argument.

205 ↗(On Diff #47280)

Maybe Lexer::getLocForEndOfToken?

clang-tidy/misc/UninitializedFieldCheck.cpp
178 ↗(On Diff #47159)

I see now. A comment explaining why this is done would be helpful here.

flx updated this revision to Diff 47590.Feb 10 2016, 8:54 PM
flx marked 3 inline comments as done.
flx added inline comments.
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
205 ↗(On Diff #47280)

Tried:

Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(),
                                 Field->getName().size(),
                                 Result.Context->getSourceManager(),
                                 Result.Context->getLangOpts()),

but that placed the {} before the field name:

  • bool G /* with comment */;

+ bool {}G /* with comment */;

flx updated this revision to Diff 47594.Feb 10 2016, 8:57 PM

Added comment that we're returning early if this constructor simply delegates to another constructor.

alexfh added inline comments.Feb 11 2016, 5:23 AM
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
206 ↗(On Diff #47594)

IIUC what this is doing, the offset should be 0 instead of Field->getName().size(). I've tried Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0, *Result.SourceManager, Result.Context->getLangOpts()) and it seems to work fine on your tests.

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

We also need to check that the macro definition stays the same. The lack of CHECK-FIXES doesn't ensure the fix wasn't applied (probably, it should, but that's another story).

flx updated this revision to Diff 47757.Feb 11 2016, 5:47 PM
flx marked 2 inline comments as done.
flx added inline comments.
clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
206 ↗(On Diff #47594)

Great, that worked.

alexfh accepted this revision.Feb 12 2016, 7:23 AM
alexfh edited edge metadata.

Looks good! Thank you for the new awesome check and to Jonathan for the original patch!

This revision is now accepted and ready to land.Feb 12 2016, 7:23 AM
jbcoe accepted this revision.Feb 12 2016, 12:44 PM
jbcoe edited edge metadata.
This revision was automatically updated to reflect the committed changes.