donat.nagy updated this revision to Diff 175502.
donat.nagy added a comment.
Remove braces, move if parent checking to ASTMatcher, other minor improvements
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54757/new/
https://reviews.llvm.org/D54757
Files:
clang-tidy/bugprone/BranchCloneCheck.cpp
clang-tidy/bugprone/BranchCloneCheck.h
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-branch-clone.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-branch-clone.cpp
Index: test/clang-tidy/bugprone-branch-clone.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,760 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+ out++;
+
+ out++;
+}
+
+void test_basic2(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ }
+ else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+ out++;
+ }
+
+ out++;
+}
+
+void test_basic3(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ }
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+ out++;
+
+ out++;
+}
+
+void test_basic4(int in, int &out) {
+ if (in > 77) {
+ out--;
+ }
+ else {
+ out++;
+ }
+}
+
+void test_basic5(int in, int &out) {
+ if (in > 77) {
+ out++;
+ }
+ else {
+ out++;
+ out++;
+ }
+}
+
+void test_basic6(int in, int &out) {
+ if (in > 77) {
+ out++;
+ }
+ else {
+ out++, out++;
+ }
+}
+
+void test_basic7(int in, int &out) {
+ if (in > 77) {
+ out++;
+ out++;
+ }
+ else
+ out++;
+
+ out++;
+}
+
+void test_basic8(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ out++;
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ out++;
+ out++;
+ }
+
+ if (in % 2)
+ out++;
+}
+
+void test_basic9(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ if (in % 2)
+ out++;
+ else
+ out--;
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ if (in % 2)
+ out++;
+ else
+ out--;
+ }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+ if (in > 77)
+ if (in % 2)
+ out++;
+ else
+ out--;
+ else
+ if (in % 2)
+ out++;
+ else
+ out--;
+
+}
+
+void test_basic11(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ if (in % 2)
+ out++;
+ else
+ out--;
+ if (in % 3)
+ out++;
+ else
+ out--;
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ if (in % 2)
+ out++;
+ else
+ out--;
+ if (in % 3)
+ out++;
+ else
+ out--;
+ }
+}
+
+void test_basic12(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ }
+}
+
+void test_basic13(int in, int &out) {
+ if (in > 77) {
+ // Empty compound statement is not identical to null statement.
+ } else;
+}
+
+
+void test_chain1(int in, int &out) {
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+ else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+ out++;
+
+ out++;
+}
+
+void test_chain2(int in, int &out) {
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+ else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+ out++;
+ else if (in > 42)
+ out--;
+ else if (in > 28)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 2 starts here
+ out++;
+ else if (in > 12) {
+ out++;
+ out *= 7;
+ } else if (in > 7) {
+// CHECK-MESSAGES: :[[@LINE-1]]:22: note: clone 3 starts here
+ out++;
+ }
+}
+
+void test_chain3(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+ out++;
+// CHECK-MESSAGES: :[[@LINE+1]]:4: note: end of the original
+ } else if (in > 55) {
+// CHECK-MESSAGES: :[[@LINE-1]]:23: note: clone 1 starts here
+ out++;
+ out++;
+ } else if (in > 42)
+ out--;
+ else if (in > 28) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: note: clone 2 starts here
+ out++;
+ out++;
+ } else if (in > 12) {
+ out++;
+ out++;
+ out++;
+ out *= 7;
+ } else if (in > 7) {
+// CHECK-MESSAGES: :[[@LINE-1]]:22: note: clone 3 starts here
+ out++;
+ out++;
+ }
+}
+
+// In this chain there are two clone families; notice that the checker
+// describes all branches of the first one before mentioning the second one.
+void test_chain4(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+ out++;
+// CHECK-MESSAGES: :[[@LINE+1]]:4: note: end of the original
+ } else if (in > 55) {
+// CHECK-MESSAGES: :[[@LINE-1]]:23: note: clone 1 starts here
+// CHECK-MESSAGES: :[[@LINE+8]]:21: note: clone 2 starts here
+// CHECK-MESSAGES: :[[@LINE+15]]:22: note: clone 3 starts here
+ out++;
+ out++;
+ } else if (in > 42)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out--;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+ else if (in > 28) {
+ out++;
+ out++;
+ } else if (in > 12) {
+ out++;
+ out++;
+ out++;
+ out *= 7;
+ } else if (in > 7) {
+ out++;
+ out++;
+ } else if (in > -3) {
+// CHECK-MESSAGES: :[[@LINE-1]]:23: note: clone 1 starts here
+ out--;
+ }
+}
+
+void test_chain5(int in, int &out) {
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+ else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+ out++;
+ else if (in > 42)
+ out--;
+ else if (in > 28)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 2 starts here
+ out++;
+ else if (in > 12) {
+ out++;
+ out *= 7;
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: clone 3 starts here
+ out++;
+ }
+}
+
+void test_chain6(int in, int &out) {
+ if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+ out++;
+// CHECK-MESSAGES: :[[@LINE+1]]:4: note: end of the original
+ } else if (in > 55) {
+// CHECK-MESSAGES: :[[@LINE-1]]:23: note: clone 1 starts here
+ out++;
+ out++;
+ } else if (in > 42)
+ out--;
+ else if (in > 28) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: note: clone 2 starts here
+ out++;
+ out++;
+ } else if (in > 12) {
+ out++;
+ out++;
+ out++;
+ out *= 7;
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: clone 3 starts here
+ out++;
+ out++;
+ }
+}
+
+void test_nested(int a, int b, int c, int &out) {
+ if (a > 5) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+27]]:5: note: else branch starts here
+ if (b > 5) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: repeated branch in conditional chain [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+9]]:6: note: end of the original
+// CHECK-MESSAGES: :[[@LINE+8]]:24: note: clone 1 starts here
+// CHECK-MESSAGES: :[[@LINE+14]]:12: note: clone 2 starts here
+ if (c > 5)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:7: note: else branch starts here
+ out++;
+ } else if (b > 15) {
+ if (c > 5)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:7: note: else branch starts here
+ out++;
+ } else {
+ if (c > 5)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:7: note: else branch starts here
+ out++;
+ }
+ } else {
+ if (b > 5) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: repeated branch in conditional chain [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+9]]:6: note: end of the original
+// CHECK-MESSAGES: :[[@LINE+8]]:24: note: clone 1 starts here
+// CHECK-MESSAGES: :[[@LINE+14]]:12: note: clone 2 starts here
+ if (c > 5)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:7: note: else branch starts here
+ out++;
+ } else if (b > 15) {
+ if (c > 5)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:7: note: else branch starts here
+ out++;
+ } else {
+ if (c > 5)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: if with identical then and else branches [bugprone-branch-clone]
+ out++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:7: note: else branch starts here
+ out++;
+ }
+ }
+}
+
+template <class T>
+void test_template_not_instantiated(const T &t) {
+ int a;
+ if (t)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ a++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+ a++;
+}
+
+template <class T>
+void test_template_instantiated(const T &t) {
+ int a;
+ if (t)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ a++;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+ a++;
+}
+
+template void test_template_instantiated<int>(const int &t);
+
+template <class T>
+void test_template2(T t, int a) {
+ if (a) {
+ T b(0);
+ a += b;
+ } else {
+ int b(0);
+ a += b;
+ }
+}
+
+template void test_template2<int>(int t, int a);
+
+template <class T>
+void test_template3(T t, int a) {
+ if (a) {
+ T b(0);
+ a += b;
+ } else {
+ int b(0);
+ a += b;
+ }
+}
+
+template void test_template3<short>(short t, int a);
+
+template <class T>
+void test_template_two_instances(T t, int &a) {
+ if (a) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ a += int(t);
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ a += int(t);
+ }
+}
+
+template void test_template_two_instances<short>(short t, int &a);
+template void test_template_two_instances<long>(long t, int &a);
+
+class C {
+ int member;
+ void inline_method(int arg) {
+ if (arg)
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: if with identical then and else branches [bugprone-branch-clone]
+ member = 3;
+ else
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ member = 3;
+ }
+ int other_method();
+};
+
+int C::other_method() {
+ if (member) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+ return 8;
+ } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+ return 8;
+ }
+}
+
+int simple_switch(char ch) {
+ switch (ch) {
+// CHECK-MESSAGES: :[[@LINE+1]]:3: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
+ case 'a':
+ return 10;
+ case 'A':
+ return 10;
+// CHECK-MESSAGES: :[[@LINE-1]]:14: note: last of these clones ends here
+// CHECK-MESSAGES: :[[@LINE+1]]:3: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
+ case 'b':
+ return 11;
+ case 'B':
+ return 11;
+// CHECK-MESSAGES: :[[@LINE-1]]:14: note: last of these clones ends here
+// CHECK-MESSAGES: :[[@LINE+1]]:3: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
+ case 'c':
+ return 10;
+ case 'C':
+ return 10;
+// CHECK-MESSAGES: :[[@LINE-1]]:14: note: last of these clones ends here
+ default:
+ return 0;
+ }
+}
+
+int long_sequence_switch(char ch) {
+ switch (ch) {
+// CHECK-MESSAGES: :[[@LINE+1]]:3: warning: switch has 7 consecutive identical branches [bugprone-branch-clone]
+ case 'a':
+ return 10;
+ case 'A':
+ return 10;
+ case 'b':
+ return 10;
+ case 'B':
+ return 10;
+ case 'c':
+ return 10;
+ case 'C':
+ return 10;
+ default:
+ return 10;
+// CHECK-MESSAGES: :[[@LINE-1]]:14: note: last of these clones ends here
+ }
+}
+
+int nested_switch(int a, int b, int c) {
+ switch (a) {
+// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+114]]:6: note: last of these clones ends here
+ case 1:
+ switch (b) {
+// CHECK-MESSAGES: :[[@LINE+2]]:5: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+33]]:8: note: last of these clones ends here
+ case 1:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ case 2:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ default:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ }
+ case 2:
+ switch (b) {
+// CHECK-MESSAGES: :[[@LINE+2]]:5: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+33]]:8: note: last of these clones ends here
+ case 1:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ case 2:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ default:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ }
+ default:
+ switch (b) {
+// CHECK-MESSAGES: :[[@LINE+2]]:5: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+// CHECK-MESSAGES: :[[@LINE+33]]:8: note: last of these clones ends here
+ case 1:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ case 2:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ default:
+ switch (c) {
+// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: switch has 3 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ return 42;
+ case 2:
+ return 42;
+ default:
+ return 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: note: last of these clones ends here
+ }
+ }
+ }
+}
+
+
+// This should not produce warnings, as in switch statements we only report
+// identical branches when they are consecutive. Also note that a branch
+// terminated by a break is different from a branch terminated by the end of
+// the switch statement.
+int interleaved_cases(int a, int b) {
+ switch (a) {
+ case 3:
+ case 4:
+ b = 2;
+ break;
+ case 5:
+ b = 3;
+ break;
+ case 6:
+ b = 2;
+ break;
+ case 7:
+ if (b % 2) {
+ b++;
+ } else {
+ b++;
+ break;
+ }
+ b = 2;
+ break;
+ case 8:
+ b = 2;
+ case 9:
+ b = 3;
+ break;
+ default:
+ b = 3;
+ }
+ return b;
+}
+
+
+// A case: or default: is only considered to be the start of a branch if it is a direct child of the CompoundStmt forming the body of the switch
+int buried_cases(int foo) {
+ switch (foo) {
+ {
+ case 36:
+ return 8;
+ default:
+ return 8;
+ }
+ }
+}
+
+// Here the `case 7:` is a child statement of the GotoLabelStmt, so the checker
+// thinks that it is part of the `case 9:` branch. While this result is
+// counterintuitve, mixing goto labels and switch statements in this fashion is
+// pretty rare, so it does not deserve a special case in the checker code.
+int decorated_cases(int z) {
+ if (!(z % 777)) {
+ goto lucky;
+ }
+ switch (z) {
+// CHECK-MESSAGES: :[[@LINE+1]]:3: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
+ case 1:
+ case 2:
+ case 3:
+ z++;
+ break;
+ case 4:
+ case 5:
+ z++;
+ break;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: last of these clones ends here
+ case 9:
+ z++;
+ break;
+ lucky:
+ case 7:
+ z += 3;
+ z *= 2;
+ break;
+ case 92:
+ z += 3;
+ z *= 2;
+ break;
+ default:
+ z++;
+ }
+ return z + 92;
+}
+
+// The child of the switch statement is not neccessarily a compound statement,
+// do not crash in this unusual case.
+char no_real_body(int in, int &out) {
+ switch (in)
+ case 42:
+ return 'A';
+
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+ out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+ else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+ out++;
+ else if (in > 34)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 2 starts here
+ out++;
+
+ return '|';
+}
+
+// Duff's device [https://en.wikipedia.org/wiki/Duff's_device]
+// The check does not try to distinguish branches in this sort of convoluted
+// code, but it should avoid crashing.
+void send(short *to, short *from, int count)
+{
+ int n = (count + 7) / 8;
+ switch (count % 8) {
+ case 0: do { *to = *from++;
+ case 7: *to = *from++;
+ case 6: *to = *from++;
+ case 5: *to = *from++;
+ case 4: *to = *from++;
+ case 3: *to = *from++;
+ case 2: *to = *from++;
+ case 1: *to = *from++;
+ } while (--n > 0);
+ }
+}
+
+// We are not checking ternary operators (for now).
+int ternary1(bool b, int x) {
+ return b ? x : x;
+}
+
+int ternary2(bool b, int x) {
+ return b ? 42 : 42;
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -9,8 +9,8 @@
abseil-no-internal-dependencies
abseil-no-namespace
abseil-redundant-strcat-calls
- abseil-string-find-startswith
abseil-str-cat-append
+ abseil-string-find-startswith
android-cloexec-accept
android-cloexec-accept4
android-cloexec-creat
@@ -28,6 +28,7 @@
bugprone-argument-comment
bugprone-assert-side-effect
bugprone-bool-pointer-implicit-conversion
+ bugprone-branch-clone
bugprone-copy-constructor-init
bugprone-dangling-handle
bugprone-exception-escape
Index: docs/clang-tidy/checks/bugprone-branch-clone.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-branch-clone.rst
@@ -0,0 +1,77 @@
+.. title:: clang-tidy - bugprone-branch-clone
+
+bugprone-branch-clone
+=====================
+
+Checks for repeated branches in ``if/else if/else`` chains and consecutive
+repeated branches in ``switch`` statements.
+
+ .. code-block:: c++
+
+ if (test_value(x)) {
+ y++;
+ do_something(x, y);
+ } else {
+ y++;
+ do_something(x, y);
+ }
+
+In this simple example (which could arise e.g. as a copy-paste error) the
+``then`` and ``else`` branches are identical and the code is equivalent the
+following shorter and cleaner code:
+
+ .. code-block:: c++
+
+ test_value(x); // can be omitted unless it has side effects
+ y++;
+ do_something(x, y);
+
+
+If this is the inteded behavior, then there is no reason to use a conditional
+statement; otherwise the issue can be solved by fixing the branch that is
+handled incorrectly.
+
+The check also detects repeated branches in longer ``if/else if/else`` chains
+where it would be even harder to notice the problem.
+
+In ``switch`` statements the check only reports repeated branches when they are
+consecutive, because it is relatively common that the ``case:`` labels have
+some natural ordering and rearranging them would decrease the readability of
+the code. For example:
+
+ .. code-block:: c++
+
+ switch (ch) {
+ case 'a':
+ return 10;
+ case 'A':
+ return 10;
+ case 'b':
+ return 11;
+ case 'B':
+ return 11;
+ default:
+ return 10;
+ }
+
+Here the checker reports that the ``'a'`` and ``'A'`` branches are identical
+(and that the ``'b'`` and ``'B'`` branches are also identical), but does not
+report that the ``default:`` branch is also idenical to the first two branches.
+If this is indeed the correct behavior, then it could be implemented as:
+
+ .. code-block:: c++
+
+ switch (ch) {
+ case 'a':
+ case 'A':
+ return 10;
+ case 'b':
+ case 'B':
+ return 11;
+ default:
+ return 10;
+ }
+
+Here the checker does not warn for the repeated ``return 10;``, which is good
+if we want to preserve that ``'a'`` is before ``'b'`` and ``default:`` is the
+last branch.
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -103,6 +103,12 @@
Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests
``absl::StrAppend()`` should be used instead.
+- New :doc:`bugprone-branch-clone
+ <clang-tidy/checks/bugprone-branch-clone>` check.
+
+ Checks for repeated branches in ``if/else if/else`` chains and consecutive
+ repeated branches in ``switch`` statements.
+
- New :doc:`modernize-concat-nested-namespaces
<clang-tidy/checks/modernize-concat-nested-namespaces>` check.
Index: clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tidy/bugprone/CMakeLists.txt
+++ clang-tidy/bugprone/CMakeLists.txt
@@ -4,6 +4,7 @@
ArgumentCommentCheck.cpp
AssertSideEffectCheck.cpp
BoolPointerImplicitConversionCheck.cpp
+ BranchCloneCheck.cpp
BugproneTidyModule.cpp
CopyConstructorInitCheck.cpp
DanglingHandleCheck.cpp
Index: clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -14,6 +14,7 @@
#include "ArgumentCommentCheck.h"
#include "AssertSideEffectCheck.h"
#include "BoolPointerImplicitConversionCheck.h"
+#include "BranchCloneCheck.h"
#include "CopyConstructorInitCheck.h"
#include "DanglingHandleCheck.h"
#include "ExceptionEscapeCheck.h"
@@ -64,6 +65,8 @@
"bugprone-assert-side-effect");
CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
"bugprone-bool-pointer-implicit-conversion");
+ CheckFactories.registerCheck<BranchCloneCheck>(
+ "bugprone-branch-clone");
CheckFactories.registerCheck<CopyConstructorInitCheck>(
"bugprone-copy-constructor-init");
CheckFactories.registerCheck<DanglingHandleCheck>(
Index: clang-tidy/bugprone/BranchCloneCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/bugprone/BranchCloneCheck.h
@@ -0,0 +1,38 @@
+//===--- BranchCloneCheck.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_BUGPRONE_BRANCHCLONECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BRANCHCLONECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// A check for detecting if/else if/else chains where two or more branches are
+/// Type I clones of each other (that is, they contain identical code) and for
+/// detecting switch statements where two or more consecutive branches are
+/// Type I clones of each other.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-branch-clone.html
+class BranchCloneCheck : public ClangTidyCheck {
+public:
+ BranchCloneCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BRANCHCLONECHECK_H
Index: clang-tidy/bugprone/BranchCloneCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -0,0 +1,203 @@
+//===--- BranchCloneCheck.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 "BranchCloneCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CloneDetection.h"
+#include "llvm/Support/Casting.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+
+/// Returns true when the statements are Type I clones of each other.
+static bool areStatementsIdentical(const Stmt *LHS, const Stmt *RHS,
+ const ASTContext &Context) {
+ llvm::FoldingSetNodeID DataLHS, DataRHS;
+ LHS->Profile(DataLHS, Context, false);
+ RHS->Profile(DataRHS, Context, false);
+ return (DataLHS == DataRHS);
+}
+
+namespace {
+/// A branch in a switch may consist of several statements; while a branch in
+/// an if/else if/else chain is one statement (which may be a CompoundStmt).
+using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
+} // anonymous namespace
+
+/// Determines if the bodies of two branches in a switch statements are Type I
+/// clones of each other. This function only examines the body of the branch
+/// and ignores the `case X:` or `default:` at the start of the branch.
+static bool areSwitchBranchesIdentical(const SwitchBranch LHS,
+ const SwitchBranch RHS,
+ const ASTContext &Context) {
+ if (LHS.size() != RHS.size())
+ return false;
+
+ for (size_t i = 0, Size = LHS.size(); i < Size; i++) {
+ // NOTE: We strip goto labels and annotations in addition to stripping
+ // the `case X:` or `default:` labels, but it is very unlikely that this
+ // would casue false positives in real-world code.
+ if (!areStatementsIdentical(LHS[i]->stripLabelLikeStatements(),
+ RHS[i]->stripLabelLikeStatements(), Context)) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ ifStmt(stmt().bind("if"),
+ hasParent(stmt(unless(ifStmt(hasElse(equalsBoundNode("if")))))),
+ hasElse(stmt().bind("else"))),
+ this);
+ Finder->addMatcher(switchStmt().bind("switch"), this);
+}
+
+void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
+ const ASTContext &Context = *Result.Context;
+
+ if (const auto *IS = Result.Nodes.getNodeAs<IfStmt>("if")) {
+ const Stmt *Then = IS->getThen();
+ assert(Then && "An IfStmt must have a `then` branch!");
+
+ // If there is no `else` branch, we may exit early because there is no
+ // chance of duplicated branches.
+ const Stmt *Else = Result.Nodes.getNodeAs<Stmt>("else");
+ if (!Else)
+ return;
+
+ if (!isa<IfStmt>(Else)) {
+ // Just a simple if with no `else if` branch.
+ if (areStatementsIdentical(Then->IgnoreContainers(),
+ Else->IgnoreContainers(), Context)) {
+ diag(IS->getBeginLoc(), "if with identical then and else branches");
+ diag(IS->getElseLoc(), "else branch starts here", DiagnosticIDs::Note);
+ }
+ return;
+ }
+
+ // This is the complicated case when we start an if/else if/else chain.
+ // To find all the duplicates, we collect all the branches into a vector.
+ llvm::SmallVector<const Stmt *, 4> Branches;
+ const IfStmt *Cur = IS;
+ while (true) {
+ // Store the `then` branch.
+ Branches.push_back(Cur->getThen());
+
+ // The chain ends if there is no `else` branch.
+ Else = Cur->getElse();
+ if (!Else)
+ break;
+
+ // Check if there is another `else if`...
+ Cur = dyn_cast<IfStmt>(Else);
+ if (!Cur) {
+ // ...this is just a plain `else` branch, concluding the chain.
+ Branches.push_back(Else);
+ break;
+ }
+ }
+
+ size_t N = Branches.size();
+ llvm::BitVector KnownAsClone(N);
+
+ for (size_t i = 0; i + 1 < N; i++) {
+ // We have already seen Branches[i] as a clone of an earlier branch.
+ if (KnownAsClone[i])
+ continue;
+
+ int NumCopies = 1;
+
+ for (size_t j = i + 1; j < N; j++) {
+ if (KnownAsClone[j] ||
+ !areStatementsIdentical(Branches[i]->IgnoreContainers(),
+ Branches[j]->IgnoreContainers(), Context))
+ continue;
+
+ NumCopies++;
+ KnownAsClone[j] = true;
+
+ if (NumCopies == 2) {
+ // We report the first occurence only when we find the second one.
+ diag(Branches[i]->getBeginLoc(),
+ "repeated branch in conditional chain");
+ diag(Lexer::getLocForEndOfToken(Branches[i]->getEndLoc(), 0,
+ *Result.SourceManager, getLangOpts()),
+ "end of the original", DiagnosticIDs::Note);
+ }
+
+ diag(Branches[j]->getBeginLoc(), "clone %0 starts here",
+ DiagnosticIDs::Note)
+ << (NumCopies - 1);
+ }
+ }
+ } else if (const auto *SS = Result.Nodes.getNodeAs<SwitchStmt>("switch")) {
+ const CompoundStmt *Body = dyn_cast_or_null<CompoundStmt>(SS->getBody());
+
+ // Code like
+ // switch (x) case 0: case 1: foobar();
+ // is legal and calls foobar() if and only if x is either 0 or 1;
+ // but we do not try to distinguish branches in such code.
+ if (!Body)
+ return;
+
+ // We will first collect the branches of the switch statements. For the
+ // sake of simplicity we say that branches are delimited by the SwitchCase
+ // (`case:` or `default:`) children of Body; that is, we ignore `case:` or
+ // `default:` labels embedded inside other statements and we do not follow
+ // the effects of `break` and other manipulation of the control-flow.
+ llvm::SmallVector<SwitchBranch, 4> Branches;
+ for (const Stmt *S : Body->body()) {
+ // If this is a `case` or `default`, we start a new, empty branch.
+ if (isa<SwitchCase>(S))
+ Branches.emplace_back();
+
+ // There may be code before the first branch (which can be dead code
+ // and can be code reached either through goto or through case labels
+ // that are embedded inside e.g. inner compound statements); we do not
+ // store those statements in branches.
+ if (!Branches.empty())
+ Branches.back().push_back(S);
+ }
+
+ auto End = Branches.end();
+ auto BeginCurrent = Branches.begin();
+ while (BeginCurrent < End) {
+ auto EndCurrent = BeginCurrent + 1;
+ while (EndCurrent < End &&
+ areSwitchBranchesIdentical(*BeginCurrent, *EndCurrent, Context)) {
+ ++EndCurrent;
+ }
+ // At this point the iterator range {BeginCurrent, EndCurrent} contains a
+ // complete family of consecutive identical branches.
+ if (EndCurrent > BeginCurrent + 1) {
+ diag(BeginCurrent->front()->getBeginLoc(),
+ "switch has %0 consecutive identical branches")
+ << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
+ diag(Lexer::getLocForEndOfToken((EndCurrent - 1)->back()->getEndLoc(), 0,
+ *Result.SourceManager, getLangOpts()),
+ "last of these clones ends here", DiagnosticIDs::Note);
+ }
+ BeginCurrent = EndCurrent;
+ }
+ } else {
+ llvm_unreachable("No if statement and no switch statement.");
+ }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
