misc-uninitialized-field

[jbcoe]

Summary

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

[mcrosier]

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]

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]

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]

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]

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

[alexfh]

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

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

95

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

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

[alexfh]

clang-tidy/misc/UninitializedFieldCheck.cpp

26

Did you miss this comment?

57

What about this?

71

Did you miss this comment?

[jbcoe]

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]

Combined pointer and builtin checks into a single condition.

[alexfh]

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]

clang-tidy/misc/UninitializedFieldCheck.cpp

57

I'll look into this.

71

agreed.

[jbcoe]

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.

[jbcoe]

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

[mgehre]

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]

In D10553#267535, @mgehre wrote:

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]

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

[jbcoe]

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

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

[jbcoe]

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

clang-tidy/misc/CMakeLists.txt

   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
+  UninitializedFieldCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UseOverrideCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
+#include "UninitializedFieldCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UseOverrideCheck.h"

         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
+    CheckFactories.registerCheck<UninitializedFieldCheck>(
+        "misc-uninitialized-field");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");
     CheckFactories.registerCheck<UnusedRAIICheck>("misc-unused-raii");

clang-tidy/misc/UninitializedFieldCheck.h

+//===--- UninitializedFieldCheck.h - clang-tidy------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNINITIALIZED_FIELD_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNINITIALIZED_FIELD_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Checks that builtin or pointer fields are initialized by explicit
+/// constructors.
+///
+/// The constructor body is not inspected. Members initialized by assignment or
+/// through function calls in the body of the constructor will result in false
+/// positives.
+///
+/// TODO: See if 'fixes' for false positives are optimized away by the compiler.
+class UninitializedFieldCheck : public ClangTidyCheck {
+public:
+  UninitializedFieldCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNINITIALIZED_FIELD_H
+

clang-tidy/misc/UninitializedFieldCheck.cpp

+//===--- UninitializedFieldCheck.cpp - clang-tidy--------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UninitializedFieldCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <unordered_set>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+namespace {
+
+static bool fieldRequiresInit(const clang::FieldDecl *F) {
+  if (F->getType()->isPointerType() || F->getType()->isBuiltinType())
+    return true;
+  return false;
+};
+
+} // namespace
+
+void UninitializedFieldCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(constructorDecl(isDefinition()).bind("ctor"), this);
+}
+
+void UninitializedFieldCheck::check(const MatchFinder::MatchResult &Result) {
+
+  if (const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor")) {
+    // TODO: Add constructor if the compiler-generated one is not sufficient.
+    if (!Ctor->isUserProvided())
+      return;
+
+    auto ParentClass = Ctor->getParent();
+    auto MemberFields = ParentClass->fields();
+
+    std::unordered_set<const clang::FieldDecl *> FieldsToInit;
+    std::copy_if(MemberFields.begin(), MemberFields.end(),
+                 std::inserter(FieldsToInit, FieldsToInit.end()),
+                 &fieldRequiresInit);
+
+    if (FieldsToInit.empty())
+      return;
+
+    for (clang::CXXCtorInitializer *Init : Ctor->inits()) {
+      const FieldDecl *MemberField = Init->getMember();
+      if (Init->isDelegatingInitializer())
+        return;
+
+      if (!MemberField)
+        continue;
+
+      FieldsToInit.erase(MemberField);
+    }
+
+    if (FieldsToInit.empty())
+      return;
+
+    std::string Replacement(":");
+    llvm::raw_string_ostream ReplacementStream(Replacement);
+    for (const auto MemberField : MemberFields)
+      if (FieldsToInit.find(MemberField) != FieldsToInit.end())
+        ReplacementStream << MemberField->getNameAsString() << "(),";
+    Replacement = ReplacementStream.str();
+
+    assert(!Replacement.empty());
+    Replacement.pop_back(); // remove trailing ','.
+
+    // Do we need a ':' or a ',' at the beginning of our new initializer list?
+    auto FirstWrittenInitializerLocation =
+        std::find_if(Ctor->inits().begin(), Ctor->inits().end(),
+                     [](const clang::CXXCtorInitializer *Initializer) {
+                       return Initializer->isWritten();
+                     });
+
+    if (FirstWrittenInitializerLocation != Ctor->inits().end())
+      Replacement[0] = ',';
+
+    diag(Ctor->getLocStart(),
+         "constructor '%0' needs to initialize all built-in or pointer fields")
+        << Ctor->getNameAsString()
+        << FixItHint::CreateInsertion(Ctor->getBody()->getLocStart(),
+                                      Replacement);
+  }
+}
+
+} // namespace tidy
+} // namespace clang

test/clang-tidy/misc-uninitialized-field.cpp

+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-uninitialized-field %t
+// REQUIRES: shell
+
+struct A
+{
+  int a_;
+
+  A() {}
+};
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: constructor 'A' needs to initialize all built-in or pointer fields
+
+
+struct B
+{
+  B() {}
+  
+  int b_;
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: constructor 'B' needs to initialize all built-in or pointer fields
+
+
+struct C
+{
+  int c_;
+
+  C();
+};
+
+C::C() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor 'C' needs to initialize all built-in or pointer fields
+
+struct D
+{
+  int d_;
+
+  D() : d_() {}
+};
+
+struct E
+{
+  int e_;
+
+  E();
+};
+E::E() : e_() {}
+
+struct F
+{
+  int f_ = 0;
+
+  F() {}
+};
+
+struct G
+{
+  int g_;
+
+  G(int g) : g_(g) {}
+  G() : G(0) {}
+};
+