Skip to content
Merged
3 changes: 3 additions & 0 deletions clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "IncorrectRoundingsCheck.h"
#include "InfiniteLoopCheck.h"
#include "IntegerDivisionCheck.h"
#include "InvalidEnumDefaultInitializationCheck.h"
#include "LambdaFunctionNameCheck.h"
#include "MacroParenthesesCheck.h"
#include "MacroRepeatedSideEffectsCheck.h"
Expand Down Expand Up @@ -164,6 +165,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<InfiniteLoopCheck>("bugprone-infinite-loop");
CheckFactories.registerCheck<IntegerDivisionCheck>(
"bugprone-integer-division");
CheckFactories.registerCheck<InvalidEnumDefaultInitializationCheck>(
"bugprone-invalid-enum-default-initialization");
CheckFactories.registerCheck<LambdaFunctionNameCheck>(
"bugprone-lambda-function-name");
CheckFactories.registerCheck<MacroParenthesesCheck>(
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ add_clang_library(clangTidyBugproneModule STATIC
InaccurateEraseCheck.cpp
IncorrectEnableIfCheck.cpp
IncorrectEnableSharedFromThisCheck.cpp
InvalidEnumDefaultInitializationCheck.cpp
UnintendedCharOstreamOutputCheck.cpp
ReturnConstRefFromParameterCheck.cpp
SuspiciousStringviewDataUsageCheck.cpp
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
//===--- InvalidEnumDefaultInitializationCheck.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 "InvalidEnumDefaultInitializationCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include <algorithm>

using namespace clang::ast_matchers;

namespace clang::tidy::bugprone {

namespace {

AST_MATCHER(EnumDecl, isCompleteAndHasNoZeroValue) {
const EnumDecl *Definition = Node.getDefinition();
return Definition && Node.isComplete() && !Node.enumerators().empty() &&
std::none_of(Definition->enumerator_begin(),
Definition->enumerator_end(),
[](const EnumConstantDecl *Value) {
return Value->getInitVal().isZero();
});
}

AST_MATCHER(Expr, isEmptyInit) {
if (isa<CXXScalarValueInitExpr>(&Node))
return true;
if (isa<ImplicitValueInitExpr>(&Node))
return true;
if (const auto *Init = dyn_cast<InitListExpr>(&Node))
return Init->getNumInits() == 0;
return false;
}

} // namespace

InvalidEnumDefaultInitializationCheck::InvalidEnumDefaultInitializationCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}

bool InvalidEnumDefaultInitializationCheck::isLanguageVersionSupported(
const LangOptions &LangOpts) const {
return LangOpts.CPlusPlus;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this check also valuable for C?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check was improved to handle initializer lists that can be used in C code too.


void InvalidEnumDefaultInitializationCheck::registerMatchers(
MatchFinder *Finder) {
Finder->addMatcher(
expr(isEmptyInit(),
hasType(hasUnqualifiedDesugaredType(enumType(hasDeclaration(
enumDecl(isCompleteAndHasNoZeroValue()).bind("enum"))))))
.bind("expr"),
this);
}

void InvalidEnumDefaultInitializationCheck::check(
const MatchFinder::MatchResult &Result) {
const auto *InitExpr = Result.Nodes.getNodeAs<Expr>("expr");
const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("enum");
if (!InitExpr || !Enum)
return;

ASTContext &ACtx = Enum->getASTContext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const? Same for Parents below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getParents requires it to be non-const.

SourceLocation Loc = InitExpr->getExprLoc();
if (Loc.isInvalid()) {
if (isa<ImplicitValueInitExpr>(InitExpr) || isa<InitListExpr>(InitExpr)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think isa<>() now supports variadic args, so we could use:
isa<ImplicitValueInitExpr, InitListExpr>(InitExpr)

DynTypedNodeList Parents = ACtx.getParents(*InitExpr);
if (Parents.empty())
return;

if (const auto *Ctor = Parents[0].get<CXXConstructorDecl>()) {
// Try to find member initializer with the found expression and get the
// source location from it.
CXXCtorInitializer *const *CtorInit = std::find_if(
Ctor->init_begin(), Ctor->init_end(),
[InitExpr](const CXXCtorInitializer *Init) {
return Init->isMemberInitializer() && Init->getInit() == InitExpr;
});
if (!CtorInit)
return;
Loc = (*CtorInit)->getLParenLoc();
} else if (const auto *InitList = Parents[0].get<InitListExpr>()) {
// The expression may be implicitly generated for an initialization.
// Search for a parent initialization list with valid source location.
while (InitList->getExprLoc().isInvalid()) {
DynTypedNodeList Parents = ACtx.getParents(*InitList);
if (Parents.empty())
return;
InitList = Parents[0].get<InitListExpr>();
if (!InitList)
return;
}
Loc = InitList->getExprLoc();
}
}
// If still not found a source location, omit the warning.
// FIXME: All such cases should be fixed to make the checker more precise.
if (Loc.isInvalid())
return;
}
diag(Loc, "enum value of type %0 initialized with invalid value of 0, "
"enum doesn't have a zero-value enumerator")
<< Enum;
diag(Enum->getLocation(), "enum is defined here", DiagnosticIDs::Note);
}

} // namespace clang::tidy::bugprone
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//===--- InvalidEnumDefaultInitializationCheck.h - clang-tidy -*- C++ -*---===//
//
// 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
//
//===----------------------------------------------------------------------===//

#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INVALIDENUMDEFAULTINITIALIZATIONCHECK_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INVALIDENUMDEFAULTINITIALIZATIONCHECK_H

#include "../ClangTidyCheck.h"

namespace clang::tidy::bugprone {

/// Detects default initialization (to 0) of variables with `enum` type where
/// the enum has no enumerator with value of 0.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/invalid-enum-default-initialization.html
class InvalidEnumDefaultInitializationCheck : public ClangTidyCheck {
public:
InvalidEnumDefaultInitializationCheck(StringRef Name,
ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is relevant for C, C++ and Objective-C languages as well, so I think you are right to not override the isLanguageVersionSupported method here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLanguageVersionSupported was already removed.


} // namespace clang::tidy::bugprone

#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INVALIDENUMDEFAULTINITIALIZATIONCHECK_H
6 changes: 6 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,12 @@ New checks
pointer and store it as class members without handle the copy and move
constructors and the assignments.

- New :doc:`bugprone-invalid-enum-default-initialization
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase from main.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not use rebase because it may remove commits and make comparison difficult. Instead a merge from main was done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge complicates history and make it non-linear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the github pull request UI gets confused when commits are rebased or amended on the branch that acts as the "source" of the PR. (IIRC inline comments will get lost, but there may be other problems as well.) For this reason I think it's prudent to use merge commits during the lifetime of a github review.

Note that when the PR is accepted all the commits on it (including the merge commits) will be squashed into a single commit that gets rebased onto the main branch -- so there is no danger of complicating the canonical history of the project. (And the local/internal history of a single PR is small enough that it will remain understandable even if there are merge commits from main.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM documentation (https://llvm.org/docs/GitHub.html) actually tells that we should rebase the PR after the review is finished, force push a final commit and merge this on github. (But rebase can involve changes that should be reviewed?) Comparing changes between PR commits (like a diff only of the last update) seems to be not possible after rebase. "Squash and merge" does work (if there is no conflict) and leaves a single (non-merge) commit in the main branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM documentation (https://llvm.org/docs/GitHub.html) actually tells that we should rebase the PR after the review is finished, force push a final commit and merge this on github.

The documentation primarily suggest using the "Squash and merge" button and only offers this "Merge using the GitHub command line interface" workflow (that you appear to summarize) as a secondary alternative. Also note that there is almost no difference between using "Squash and merge" and this CLI workflow.

(But rebase can involve changes that should be reviewed?)

Yes, some rebases can involve nontrivial changes, but the intended workflow is that:

  • if the change is not yet compatible with the main branch (a rebase would involve nontrivial changes), then merge the main branch to the PR branch, add the necessary correction commits and review them;
  • once the change is complete and compatible with the main branch (i.e. it can be trivially rebased), it should be squashed and rebased (either by the slightly misnamed "Squash and merge" button, or the equivalent CLI workflow) -- and in this situation the rebase is trivial.

Comparing changes between PR commits (like a diff only of the last update) seems to be not possible after rebase.

Yes, that's a significant reason why rebases during the lifetime of the PR should be avoided.

"Squash and merge" does work (if there is no conflict) and leaves a single (non-merge) commit in the main branch.

Yes, it's a bit unfortunate that they called it "Squash and merge" instead of "Squash and rebase" (which would accurately describe its effects) -- but it works correctly.

<clang-tidy/checks/bugprone/invalid-enum-default-initialization>` check.

Detects default initialization (to 0) of variables with ``enum`` type where
the enum has no enumerator with value of 0.

- New :doc:`bugprone-unintended-char-ostream-output
<clang-tidy/checks/bugprone/unintended-char-ostream-output>` check.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
.. title:: clang-tidy - bugprone-invalid-enum-default-initialization

bugprone-invalid-enum-default-initialization
============================================

Detects default initialization (to 0) of variables with ``enum`` type where
the enum has no enumerator with value of 0.

In C++ a default initialization is performed if a variable is initialized with
initializer list or in other implicit ways, and no value is specified at the
initialization. In such cases the value 0 is used for the initialization.
This also applies to enumerations even if it does not have an enumerator with
value 0. In this way a variable with the enum type may contain initially an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
value 0. In this way a variable with the enum type may contain initially an
value 0. In this way a variable with the ``enum`` type may contain initially an

invalid value (if the program expects that it contains only the listed
enumerator values).

The checker emits a warning only if an enum variable is default-initialized
(contrary to not initialized) and the enum type does not have an enumerator with
value of 0. The enum type can be scoped or non-scoped enum.

.. code-block:: c++

enum class Enum1: int {
A = 1,
B
};

enum class Enum0: int {
A = 0,
B
};

void f() {
Enum1 X1{}; // warn: 'X1' is initialized to 0
Enum1 X2 = Enum1(); // warn: 'X2' is initialized to 0
Enum1 X3; // no warning: 'X3' is not initialized
Enum0 X4{}; // no warning: type has an enumerator with value of 0
}

struct S1 {
Enum1 A;
S(): A() {} // warn: 'A' is initialized to 0
};

struct S2 {
int A;
Enum1 B;
};

S2 VarS2{}; // warn: member 'B' is initialized to 0
1 change: 1 addition & 0 deletions clang-tools-extra/docs/clang-tidy/checks/list.rst
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ Clang-Tidy Checks
:doc:`bugprone-incorrect-roundings <bugprone/incorrect-roundings>`,
:doc:`bugprone-infinite-loop <bugprone/infinite-loop>`,
:doc:`bugprone-integer-division <bugprone/integer-division>`,
:doc:`bugprone-invalid-enum-default-initialization <bugprone/invalid-enum-default-initialization>`,
:doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`,
:doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes"
:doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// RUN: %check_clang_tidy -std=c++17 %s bugprone-invalid-enum-default-initialization %t

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test with enum forward declaration only, and with enum without enumerators.
For those there should be no requirement in initialization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are added now.

enum class Enum0: int {
A = 0,
B
};

enum class Enum1: int {
A = 1,
B
};

enum Enum2 {
Enum_A = 4,
Enum_B
};

Enum0 E0_1{};
Enum0 E0_2 = Enum0();
Enum0 E0_3;
Enum0 E0_4{0};
Enum0 E0_5{Enum0::A};
Enum0 E0_6{Enum0::B};

Enum1 E1_1{};
// CHECK-NOTES: :[[@LINE-1]]:11: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 E1_2 = Enum1();
// CHECK-NOTES: :[[@LINE-1]]:14: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 E1_3;
Enum1 E1_4{0};
Enum1 E1_5{Enum1::A};
Enum1 E1_6{Enum1::B};

Enum2 E2_1{};
// CHECK-NOTES: :[[@LINE-1]]:11: warning: enum value of type 'Enum2' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :13:6: note: enum is defined here
Enum2 E2_2 = Enum2();
// CHECK-NOTES: :[[@LINE-1]]:14: warning: enum value of type 'Enum2' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :13:6: note: enum is defined here

void f1() {
static Enum1 S; // FIMXE: warn for this?
Enum1 A;
Enum1 B = Enum1();
// CHECK-NOTES: :[[@LINE-1]]:13: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
int C = int();
}

void f2() {
Enum1 A{};
// CHECK-NOTES: :[[@LINE-1]]:10: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 B = Enum1();
// CHECK-NOTES: :[[@LINE-1]]:13: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 C[5] = {{}};
// CHECK-NOTES: :[[@LINE-1]]:17: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 D[5] = {}; // FIMXE: warn for this?
}

struct S1 {
Enum1 E_1{};
// CHECK-NOTES: :[[@LINE-1]]:12: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 E_2 = Enum1();
// CHECK-NOTES: :[[@LINE-1]]:15: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
Enum1 E_3;
Enum1 E_4;
Enum1 E_5;

S1() :
E_3{},
// CHECK-NOTES: :[[@LINE-1]]:8: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
E_4(),
// CHECK-NOTES: :[[@LINE-1]]:8: warning: enum value of type 'Enum1' initialized with invalid value of 0, enum doesn't have a zero-value enumerator
// CHECK-NOTES: :8:12: note: enum is defined here
E_5{Enum1::B}
{}
};

struct S2 {
Enum0 X;
Enum1 Y;
Enum2 Z;
};

struct S3 {
S2 X;
int Y;
};

struct S4 : public S3 {
int Z;
};

S2 VarS2{};
// CHECK-NOTES: :[[@LINE-1]]:9: warning: enum value of type 'Enum1' initialized with invalid value of 0
// CHECK-NOTES: :8:12: note: enum is defined here
// CHECK-NOTES: :[[@LINE-3]]:9: warning: enum value of type 'Enum2' initialized with invalid value of 0
// CHECK-NOTES: :13:6: note: enum is defined here
S3 VarS3{};
// CHECK-NOTES: :[[@LINE-1]]:10: warning: enum value of type 'Enum1' initialized with invalid value of 0
// CHECK-NOTES: :8:12: note: enum is defined here
// CHECK-NOTES: :[[@LINE-3]]:10: warning: enum value of type 'Enum2' initialized with invalid value of 0
// CHECK-NOTES: :13:6: note: enum is defined here
S4 VarS4{};
// CHECK-NOTES: :[[@LINE-1]]:10: warning: enum value of type 'Enum1' initialized with invalid value of 0
// CHECK-NOTES: :8:12: note: enum is defined here
// CHECK-NOTES: :[[@LINE-3]]:10: warning: enum value of type 'Enum2' initialized with invalid value of 0
// CHECK-NOTES: :13:6: note: enum is defined here

enum class EnumFwd;

EnumFwd Fwd{};

enum class EnumEmpty {};

EnumEmpty Empty{};

template<typename T>
struct Templ {
T Mem1{};
// CHECK-NOTES: :[[@LINE-1]]:9: warning: enum value of type 'Enum1' initialized with invalid value of 0
// CHECK-NOTES: :8:12: note: enum is defined here
};

Templ<Enum1> TemplVar;
Loading