[clang-tools-extra] [clang-tidy] The first PR our of many PRs for the "Initialized Class Members" check. (PR #65189)

[via cfe-commits]

================
@@ -0,0 +1,105 @@
+//===--- CppInitClassMembersCheck.cpp - clang-tidy ------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <string>
+
+#include "CppInitClassMembersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::google {
+
+namespace {
+
+// Matches records that have a default constructor.
+AST_MATCHER(CXXRecordDecl, hasDefaultConstructor) {
+  return Node.hasDefaultConstructor();
+}
+
+// Returns the names of `Fields` in a comma separated string.
+std::string
+toCommaSeparatedString(const SmallVector<const FieldDecl *, 16> &Fields) {
+  std::string Buffer;
+  llvm::raw_string_ostream OS(Buffer);
+  llvm::interleave(
+      Fields, OS, [&OS](const FieldDecl *Decl) { OS << Decl->getName(); },
+      ", ");
+  return Buffer;
+}
+
+// Returns `true` for field types that should be reported (if additional
+// conditions are also met). For example, returns `true` for `int` because an
+// uninitialized `int` field can contain uninitialized values.
+bool shouldReportThisFieldType(QualType Ty) {
+  if (Ty.isNull())
+    return false;
+
+  // FIXME: For now, this checker focuses on several allowlisted types. We will
+  // expand coverage in future.
+  return Ty->isIntegerType() || Ty->isBooleanType();
+}
+
+} // anonymous namespace
+
+void CppInitClassMembersCheck::checkMissingMemberInitializer(
+    ASTContext &Context, const CXXRecordDecl &ClassDecl,
+    const CXXConstructorDecl *Ctor) {
+  SmallVector<const FieldDecl *, 16> FieldsToReport;
+
+  for (const FieldDecl *F : ClassDecl.fields()) {
+    if (shouldReportThisFieldType(F->getType()) && !F->hasInClassInitializer())
+      FieldsToReport.push_back(F);
+  }
+
+  if (FieldsToReport.empty())
+    return;
+
+  DiagnosticBuilder Diag =
+      diag(Ctor ? Ctor->getBeginLoc() : ClassDecl.getLocation(),
+           "%select{these fields should be initialized|constructor should "
+           "initialize these fields}0: %1")
+      << (Ctor != nullptr) << toCommaSeparatedString(FieldsToReport);
+
+  // FIXME: generate fixes.
+}
+
+void CppInitClassMembersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(cxxConstructorDecl(isDefinition(), isDefaultConstructor(),
+                                        unless(isUserProvided()))
+                         .bind("ctor"),
+                     this);
+
+  Finder->addMatcher(cxxRecordDecl(isDefinition(), hasDefaultConstructor(),
----------------
adriannistor wrote:

I don't understand this. Why is `"may opose [sic] them self"` a problem? If they sometimes oppose themselves, the condition is false and the matcher is not added. The important thing is to not oppose themselves always (in which case the condition would always be false), but that is not the case here.

We can refine this condition later.

For now, just saying "is default constructor but has no declaration in the AST" is good enough (catches a lot of our cases). We can refine later if we have data (false positives or false negatives).

My understanding is that you find this condition too broad? In that case, that is good. Because that will allow us to see false positives (and filter them out). If the condition would be too narrow, we would have false negatives. False negatives are a problem, because we may not be aware that we have them.

In short, let's leave this as it is for now and we can refine later based on data.

Thanks!

https://github.com/llvm/llvm-project/pull/65189