[clang-tidy] Add new check bugprone-tagged-union-member-count #89925
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This patch introduces a new check to find mismatches between the number of data members in a union and the number enum values present in variant-like structures. Variant-like types can look something like this: ```c++ struct variant { enum { tag1, tag2, } kind; union { int i; char c; } data; }; ``` The kind data member of the variant is supposed to tell which data member of the union is valid, however if there are fewer enum values than union members, then it is likely a mistake. The opposite is not that obvious, because it might be fine to have more enum values than union data members, but for the time being I am curious how many real bugs can be caught if we give a warning regardless. This patch also contains a heuristic where we try to guess whether the last enum constant is actually supposed to be a tag value for the variant or whether it is just holding how many enum constants have been created. I am still collecting data on how many bugs this check can catch on open source software and I will update the pull-request with the results. 
| Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
Member
| @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: None (tigbr) ChangesThis patch introduces a new check to find mismatches between the number of data members in a union and the number enum values present in variant-like structures. Variant-like types can look something like this: struct variant { enum { tag1, tag2, } kind; union { int i; char c; } data; };The kind data member of the variant is supposed to tell which data member of the union is valid, however if there are fewer enum values than union members, then it is likely a mistake. The opposite is not that obvious, because it might be fine to have more enum values than union data members, but for the time being I am curious how many real bugs can be caught if we give a warning regardless. This patch also contains a heuristic where we try to guess whether the last enum constant is actually supposed to be a tag value for the variant or whether it is just holding how many enum constants have been created. I am still collecting data on how many bugs this check can catch on open source software and I will update the pull-request with the results. Full diff: https://github.com/llvm/llvm-project/pull/89925.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 2931325d8b5798..d9e094e11bdf09 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -76,6 +76,7 @@ #include "SuspiciousStringviewDataUsageCheck.h" #include "SwappedArgumentsCheck.h" #include "SwitchMissingDefaultCaseCheck.h" +#include "TaggedUnionMemberCountCheck.h" #include "TerminatingContinueCheck.h" #include "ThrowKeywordMissingCheck.h" #include "TooSmallLoopVariableCheck.h" @@ -223,6 +224,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-suspicious-stringview-data-usage"); CheckFactories.registerCheck<SwappedArgumentsCheck>( "bugprone-swapped-arguments"); + CheckFactories.registerCheck<TaggedUnionMemberCountCheck>( + "bugprone-tagged-union-member-count"); CheckFactories.registerCheck<TerminatingContinueCheck>( "bugprone-terminating-continue"); CheckFactories.registerCheck<ThrowKeywordMissingCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 081ba67efe1538..5e7fa08aece02e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -71,6 +71,7 @@ add_clang_library(clangTidyBugproneModule SuspiciousSemicolonCheck.cpp SuspiciousStringCompareCheck.cpp SwappedArgumentsCheck.cpp + TaggedUnionMemberCountCheck.cpp TerminatingContinueCheck.cpp ThrowKeywordMissingCheck.cpp TooSmallLoopVariableCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp new file mode 100644 index 00000000000000..6a7b5a92902c86 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp @@ -0,0 +1,125 @@ +//===--- TaggedUnionMemberCountCheck.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 "TaggedUnionMemberCountCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +#include "clang/AST/PrettyPrinter.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/Support/Casting.h" +#include <limits> + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +void TaggedUnionMemberCountCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + recordDecl( + allOf(isStruct(), + has(fieldDecl(hasType(recordDecl(isUnion()).bind("union")))), + has(fieldDecl(hasType(enumDecl().bind("tags")))))) + .bind("root"), + this); +} + +static bool hasMultipleUnionsOrEnums(const RecordDecl *rec) { + int tags = 0; + int unions = 0; + for (const FieldDecl *r : rec->fields()) { + TypeSourceInfo *info = r->getTypeSourceInfo(); + QualType qualtype = info->getType(); + const Type *type = qualtype.getTypePtr(); + if (type->isUnionType()) + unions += 1; + else if (type->isEnumeralType()) + tags += 1; + if (tags > 1 || unions > 1) + return true; + } + return false; +} + +static int64_t getNumberOfValidEnumValues(const EnumDecl *ed) { + int64_t maxTagValue = std::numeric_limits<int64_t>::min(); + int64_t minTagValue = std::numeric_limits<int64_t>::max(); + + // Heuristic for counter enum constants. + // + // enum tag_with_counter { + // tag1, + // tag2, + // tag_count, <-- Searching for these enum constants + // }; + // + // The 'ce' prefix is used to abbreviate counterEnum. + // The final tag count is decreased by 1 if and only if: + // 1. The number of counting enum constants = 1, + int ceCount = 0; + // 2. The counting enum constant is the last enum constant that is defined, + int ceFirstIndex = 0; + // 3. The value of the counting enum constant is the largest out of every enum constant. + int64_t ceValue = 0; + + int64_t enumConstantsCount = 0; + for (auto En : llvm::enumerate(ed->enumerators())) { + enumConstantsCount += 1; + + int64_t enumValue = En.value()->getInitVal().getExtValue(); + StringRef enumName = En.value()->getName(); + + if (enumValue > maxTagValue) + maxTagValue = enumValue; + if (enumValue < minTagValue) + minTagValue = enumValue; + + if (enumName.ends_with_insensitive("count")) { + if (ceCount == 0) { + ceFirstIndex = En.index(); + } + ceValue = enumValue; + ceCount += 1; + } + } + + int64_t validValuesCount = maxTagValue - minTagValue + 1; + if (ceCount == 1 && + ceFirstIndex == enumConstantsCount - 1 && + ceValue == maxTagValue) { + validValuesCount -= 1; + } + return validValuesCount; +} + +void TaggedUnionMemberCountCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *root = Result.Nodes.getNodeAs<RecordDecl>("root"); + const auto *unionMatch = Result.Nodes.getNodeAs<RecordDecl>("union"); + const auto *tagMatch = Result.Nodes.getNodeAs<EnumDecl>("tags"); + + if (hasMultipleUnionsOrEnums(root)) + return; + + int64_t unionMemberCount = llvm::range_size(unionMatch->fields()); + int64_t tagCount = getNumberOfValidEnumValues(tagMatch); + + // FIXME: Maybe a emit a note when a counter enum constant was found. + if (unionMemberCount > tagCount) { + diag(root->getLocation(), "Tagged union has more data members than tags! " + "Data members: %0 Tags: %1") + << unionMemberCount << tagCount; + } else if (unionMemberCount < tagCount) { + diag(root->getLocation(), "Tagged union has fewer data members than tags! " + "Data members: %0 Tags: %1") + << unionMemberCount << tagCount; + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h b/clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h new file mode 100644 index 00000000000000..bde42da23ff38f --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h @@ -0,0 +1,31 @@ +//===--- TaggedUnionMemberCountCheck.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_TAGGEDUNIONMEMBERCOUNTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TAGGEDUNIONMEMBERCOUNTCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +// Gives warnings for tagged unions, where the number of tags is +// different from the number of data members inside the union. +// +// For the user-facing documentation see: +// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/tagged-union-member-count.html +class TaggedUnionMemberCountCheck : public ClangTidyCheck { +public: + TaggedUnionMemberCountCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TAGGEDUNIONMEMBERCOUNTCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 78b09d23d4427f..9f9069a8d08923 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -129,6 +129,12 @@ New checks Replaces certain conditional statements with equivalent calls to ``std::min`` or ``std::max``. +- New :doc:`bugprone-tagged-union-member-count + <clang-tidy/checks/bugprone/tagged-union-member-count>` check. + + Warns about suspicious looking tagged unions where the number of enum + constants and the number of union data members are not equal. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst new file mode 100644 index 00000000000000..7eac93c6c96021 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst @@ -0,0 +1,66 @@ +.. title:: clang-tidy - bugprone-tagged-union-member-count + +bugprone-tagged-union-member-count +================================== + +Gives warnings for tagged unions, where the number of tags is +different from the number of data members inside the union. + +A struct or a class is considered to be a tagged union if it has +exactly one union data member and exactly one enum data member and +any number of other data members that are neither unions or enums. + +Example +------- + +.. code-block:: c++ + + enum tags2 { + tag1, + tag2, + }; + + struct taggedunion { // warning: Tagged union has more data members than tags! Data members: 3 Tags: 2 [bugprone-tagged-union-member-count] + enum tags2 kind; + union { + int i; + float f; + char *str; + } data; + }; + + enum tags4 { + tag1, + tag2, + tag3, + tag4, + }; + + struct taggedunion { // warning: Tagged union has more fewer members than tags! Data members: 3 Tags: 4 [bugprone-tagged-union-member-count] + enum tags4 kind; + union { + int i; + float f; + char *str; + } data; + }; + +Counting enum constant heuristic +-------------------------------- + +Some programmers use enums in such a way, where the value of the last enum +constant is used to keep track of how many enum constants have been declared. + +.. code-block:: c++ + + enum tags_with_counter { + tag1, // is 0 + tag2, // is 1 + tag3, // is 2 + tags_count, // is 3 + }; + +The checker detects this usage pattern heuristically and does not include +the counter enum constant in the final tag count, since the counter is not +meant to indicate the valid variant member. + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 79e81dd174e4f3..3058bee1357fc9 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -142,6 +142,7 @@ Clang-Tidy Checks :doc:`bugprone-suspicious-stringview-data-usage <bugprone/suspicious-stringview-data-usage>`, :doc:`bugprone-swapped-arguments <bugprone/swapped-arguments>`, "Yes" :doc:`bugprone-switch-missing-default-case <bugprone/switch-missing-default-case>`, + :doc:`bugprone-tagged-union-member-count <bugprone/tagged-union-member-count>`, "Yes" :doc:`bugprone-terminating-continue <bugprone/terminating-continue>`, "Yes" :doc:`bugprone-throw-keyword-missing <bugprone/throw-keyword-missing>`, :doc:`bugprone-too-small-loop-variable <bugprone/too-small-loop-variable>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp new file mode 100644 index 00000000000000..91165f1bd64e08 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp @@ -0,0 +1,181 @@ +// RUN: %check_clang_tidy %s bugprone-tagged-union-member-count %t + +enum tags3 { +tags3_1, +tags3_2, +tags3_3, +}; + +enum tags4 { +tags4_1, +tags4_2, +tags4_3, +tags4_4, +}; + +enum tags5 { +tags5_1, +tags5_2, +tags5_3, +tags5_4, +tags5_5, +}; + +union union3 { +short *shorts; +int *ints; +float *floats; +}; + +union union4 { +short *shorts; +double *doubles; +int *ints; +float *floats; +}; + +// It is not obvious which enum is the tag for the union. +struct taggedunion2 { // No warnings expected. +enum tags3 tagA; +enum tags4 tagB; +union union4 data; +}; + +// It is not obvious which union does the tag belong to. +struct taggedunion4 { // No warnings expected. +enum tags3 tag; +union union3 dataB; +union union3 dataA; +}; + +struct taggedunion1 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tags3 tag; + union union4 data; +}; + +struct taggedunion5 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tags3 tag; + union { +int *ints; +char characters[13]; +struct { +double re; +double im; +} complex; +long l; + } data; +}; + +struct taggedunion7 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum { +tag1, +tag2, +tag3, +} tag; +union union4 data; +}; + +struct taggedunion8 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum { +tag1, +tag2, +tag3, +} tag; +union { +int *ints; +char characters[13]; +struct { +double re; +double im; +} complex; +long l; +} data; +}; + +struct nested1 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tags3 tag; +union { +char c; +short s; +int i; +struct { // CHECK-MESSAGES: :[[@LINE]]:3: warning: Tagged union has fewer data members than tags! Data members: 4 Tags: 5 [bugprone-tagged-union-member-count] +enum tags5 tag; +union union4 data; +} inner; +} data; +}; + +struct nested2 { +enum tags3 tag; +union { +float f; +int i; +struct { // CHECK-MESSAGES: :[[@LINE]]:3: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tags3 tag; +union union4 data; +} inner; +} data; +}; + +struct nested3 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has fewer data members than tags! Data members: 2 Tags: 3 [bugprone-tagged-union-member-count] +enum tags3 tag; +union { +float f; +int i; +struct innerdecl { // CHECK-MESSAGES: :[[@LINE]]:10: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tags3 tag; +union union4 data; +}; +} data; +}; + +enum tag_with_counter_lowercase { +node_type_loop, +node_type_branch, +node_type_function, +node_type_count, +}; + +enum tag_with_counter_uppercase { +NODE_TYPE_LOOP, +NODE_TYPE_BRANCH, +NODE_TYPE_FUNCTION, +NODE_TYPE_COUNT, +}; + +struct taggedunion10 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tag_with_counter_lowercase tag; +union union4 data; +}; + +struct taggedunion11 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has more data members than tags! Data members: 4 Tags: 3 [bugprone-tagged-union-member-count] +enum tag_with_counter_uppercase tag; +union union4 data; +}; + +// Without the counter enum constant the number of tags +// and the number data members are equal. +struct taggedunion12 { // No warnings expected. +enum tag_with_counter_uppercase tag; +union union3 data; +}; + +// Technically this means that every enum value is defined from 0-256 and therefore a warning is given. +enum mycolor { +mycolor_black = 0x00, +mycolor_gray = 0xcc, +mycolor_white = 0xff, +}; + +struct taggedunion9 { // CHECK-MESSAGES: :[[@LINE]]:8: warning: Tagged union has fewer data members than tags! Data members: 3 Tags: 256 [bugprone-tagged-union-member-count] +enum mycolor tag; +union { +int a; +float b; +struct { +double re; +double im; +} complex; +} data; +}; + |
Szelethus requested review from EugeneZelenko, HerrCai0907, PiotrZSL and isuckatcs 
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Show resolved Hide resolved
PiotrZSL requested changes Apr 24, 2024
Member
PiotrZSL left a comment
PiotrZSL left a comment There was a problem hiding this comment.
This check may not have to many users, but that's fine.
I'm 100% sure that I seen such structures already.
As for code changes, please fix pointed out nits and after that checkout probably could be merged.
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
isuckatcs requested changes Apr 24, 2024
Member
isuckatcs left a comment
isuckatcs left a comment There was a problem hiding this comment.
I like the motivation behind the check, but I would also like to see more testing and some formatting to be fixed.
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count.cpp Show resolved Hide resolved
whisperity requested changes May 23, 2024
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/tagged-union-member-count-strictmode.cpp Outdated Show resolved Hide resolved
Feature changes: * Added C++ classes to matcher * Added multiple prefix/suffix option support to the counting enum heuristic Implementation changes: * Distinct enum values are counted with a set instead of the previous max - min + 1 approach * Use APSInt to avoid truncation or overflow Testing changes: * Tests use more descriptive names * Removed redundant suffix from test case messages * Separated test files for C, C++, Objective-C and Objective-C++ * C++ tests are ran with the -std=c98-or-later flag * C++ tests are extended with more language feature test cases * Created tests for the Prefix/suffix feature of the counting enum heuristic * StrictMode and heuristic enabling/disabling is more thoroughly tested Also made plenty of small improvements as well.
Contributor Author
| Thank you everyone for your suggestions and observations, they were most helpful. I listed the important changes topically in the message of the latest commit. |
Contributor
| Please resolve comments that already fixed. |
5chmidti reviewed Jul 21, 2024
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
Implementation: * Removed redundant allOf from matcher. * Removed unused llvm::enumerate from loop. * Changed static_cast to llvm::dyn_cast_or_null. * Update diagnostic message so that is starts with lower case. Documentation: * Rephrased confusing parts of the documentation. * Updated the diagnostic messages in the documentation examples to start with lowercase. * Fixed the example for the prefixes in the documentation. New configuration diagnostic: Implemented a diagnostic warning for the configuration of the counting enum heuristic. When the heuristic is disabled and a prefix or a suffix is explicitly set in the configuration file a warning is emitted. Testing: * Created test case for macros. * Created test case for templates where the union and tag are not templated, but some other data member is. * Moved enum counter heuristic tests from .c and .m files into its deticated test file. * Created test for the new configuration diagnostic. * Updated tagged-union-member-count-counting-enum-heuristic-is-disabled.cpp so that the new configuration diagnostic does not trigger on this file Style and formatting: * Removed braces from single statement if statements. * Changed a couple variables to use the CamelCase naming style. * Changed diagnostic message to start with lower case. * Moved the option name string constants to the .cpp file. * Renamed StrictModeIsEnabled option to just StrictMode. * Use true and false instead of 1 and 0 in the tests of the configuration options * Renamed CountingEnumHeuristicIsEnabled to EnableCountingEnumHeuristic (code, docs, tests). * Keep text in 80 column limit in docs .rst file.
counting enum heuristic.
whisperity requested changes Aug 2, 2024
Member
whisperity left a comment
whisperity left a comment There was a problem hiding this comment.
I found a very convoluted and somewhat silly edge case where I was able to defeat the matchers to make them produce a match in a position where it is clearly not intended. So the matchers will need to be given some thought, my idea is to have them constricted in a way to report only into non-implicit code, but perhaps this needs some more elaboration.
Lambdas implicitly compile down to an unnamed CXXRecordDecl, and if you have captures in a lambda, they become (unnamed) fields of this class.
https://godbolt.org/z/rvfY5K18T
enum E { A }; union U { int A; }; int main() { enum E e; union U u; auto L = [e, u] () {}; }Match #1: <source>:9:14: note: "root" binds here 9 | auto L = [e, u] () {}; | ^ <source>:9:15: note: "tags" binds here 9 | auto L = [e, u] () {}; | ^ <source>:9:18: note: "union" binds here 9 | auto L = [e, u] () {}; | ^ clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
isuckatcs requested changes Aug 7, 2024
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
whisperity reviewed Aug 26, 2024
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
Contributor
| My say doesn't quite have the same weight here are many others, but I see no problems with this patch after so many rounds of reviews -- would it be okay to merge? |
Szelethus requested review from 5chmidti, EugeneZelenko, Szelethus and whisperity clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/tagged-union-member-count.rst Outdated Show resolved Hide resolved
HerrCai0907 approved these changes Sep 19, 2024
Contributor
HerrCai0907 left a comment
HerrCai0907 left a comment There was a problem hiding this comment.
In general LGTM.
Please fix some nits
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp Outdated Show resolved Hide resolved
whisperity approved these changes Sep 20, 2024
Szelethus approved these changes Oct 1, 2024
Contributor
Szelethus left a comment
Szelethus left a comment There was a problem hiding this comment.
Since the PR recieved two accepts from well-established tidy contributors, and from what I can see all comments have been adequetly answered, I say we should close this out!
LGTM, well done!
| @tigbr Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This patch introduces a new check to find mismatches between the number of data members in a union and the number enum values present in variant-like structures.
Variant-like types can look something like this:
The kind data member of the variant is supposed to tell which data member of the union is valid, however if there are fewer enum values than union members, then it is likely a mistake.
The opposite is not that obvious, because it might be fine to have more enum values than union data members, but for the time being I am curious how many real bugs can be caught if we give a warning regardless.
This patch also contains a heuristic where we try to guess whether the last enum constant is actually supposed to be a tag value for the variant or whether it is just holding how many enum constants have been created.
I am still collecting data on how many bugs this check can catch on open source software and I will update the pull-request with the results.