[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE360779: [clang-tidy] new check: bugprone-branch-clone (authored by Szelethus, committed by ). Changed prior to commit: https://reviews.llvm.org/D54757?vs=198025&id=199611#toc Repository: rCTE Clang

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. Did you run the new version over a big project like llvm again? If yes LGTM from my side, if not please do that before committing to ensure no new/other problems creeped in. CHANGES SIN

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus requested review of this revision. Szelethus marked an inline comment as done. Szelethus added a comment. I changed the testfiles quite a bit, surprisingly, I'd appreciate another accept on this one please :) Seems to me that the changes describe the actual reports far better... ===

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus updated this revision to Diff 198025. Szelethus added a comment. FIx the above mentioned crash. diff --git a/clang-tidy/bugprone/BranchCloneCheck.cpp b/clang-tidy/bugprone/BranchCloneCheck.cpp index f371231..a898311 100644 --- a/clang-tidy/bugprone/BranchCloneCheck.cpp +++ b/cl

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus added a comment. $ cat orig.cpp #define a(b) switch (b) { #define c(b) \ b: int d; e() { a(d) c(2); c(3) $ debugBuild/bin/clang-tidy orig.cpp -checks=bugprone-branch-clone -- clang-tidy: ../extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:200: clang::DiagnosticBuild

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus added a comment. I actually managed to reproduce the crash. extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:200: clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, DiagnosticIDs::Level): Assertion `Loc.isValid()' f

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus commandeered this revision. Szelethus added a reviewer: donat.nagy. Szelethus added a comment. I'll take a look at this. In D54757#1319831 , @JonasToth wrote: > I had to revert this patch because it broke (at least one) buildbot with an > asser

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus reopened this revision. Szelethus added a comment. This revision is now accepted and ready to land. Herald added a project: clang. Reverted in rCTE348344 . Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. I had to revert this patch because it broke (at least one) buildbot with an assertion-failure (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40436/steps/test/logs/stdio) Could you please take a look at it? I could not reproduce lo

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE348343: [clang-tidy] new check: bugprone-branch-clone (authored by JonasToth, committed by ). Changed prior to commit: https://reviews.llvm.org/D54757?vs=176408&id=176772#toc Repository: rCTE Clang

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. > A quick, tangentially related idea: Would it be useful to implement multiline > nolint sections (e.g. `//BEGINNOLINT` – `//ENDNOLINT` or something similar)? > This would be helpful for using clang-tidy on projects that contain some > automatically generated fragment

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy added a comment. Macro-generated long switches: I cannot access the check results right now, but if I recall correctly, the check assigned these errors to certain lines in the `.inc` files. This means that (1) it is not very easy to to suppress them with `//NOLINT` comments, but (2)

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. In D54757#1316899 , @donat.nagy wrote: > I applied this check to the llvm + clang codebase, and I was surprised to see > that it produced about 600 results (for comparison, the clang-tidy checks > which are enabled by default p

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 2 inline comments as done. donat.nagy added a comment. I applied this check to the llvm + clang codebase, and I was surprised to see that it produced about 600 results (for comparison, the clang-tidy checks which are enabled by default produce approximately 6000 results togethe

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 176408. donat.nagy added a comment. Minor correction in documentation 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.cp

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Eugene.Zelenko added a comment. Please close PR30233 after committing patch. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757 ___ cfe-commits mailing list cfe-com

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. LGTM, could you please run this check over real world code and check that there are no obvious false positives? Comment at: docs/clang-tidy/checks/bugprone-branch-clone.rst:10 + + .. code-block:: c++ + please do not indent the `.. c

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 8 inline comments as done. donat.nagy added a comment. At the end of the documentation I stated that the check examines the code after macro expansion. Is this note clear enough? Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4 +void test_basic1(int in

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 176066. donat.nagy added a comment. Handle ternary operators, improve documentation Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757 Files: clang-tidy/bugprone/Branc

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Eugene.Zelenko added inline comments. Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4 +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] dona

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked an inline comment as done. donat.nagy added inline comments. Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4 +void test_basic1(int in, int &out) { + if (in > 77) +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bug

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added inline comments. Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:31 +/// an if/else if/else chain is one statement (which may be a CompoundStmt). +using SwitchBranch = llvm::SmallVector; +} // anonymous namespace donat.nagy wrote: > JonasToth

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. In D54757#1311516 , @Szelethus wrote: > In D54757#1311468 , @donat.nagy > wrote: > > > **Macros:** > > > > The current implementation of the check only looks at the preprocessed > > code

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Szelethus added a comment. In D54757#1311468 , @donat.nagy wrote: > **Macros:** > > The current implementation of the check only looks at the preprocessed code, > therefore it detects the situations where the duplication is created by > macros. I added s

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 175701. donat.nagy added a comment. Correct a typo (ELSE instead of else) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757 Files: clang-tidy/bugprone/BranchCloneChec

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 3 inline comments as done. donat.nagy added a comment. **AstDiff:** I looked at the AstDiff library, but I think that it is overkill for my goals. The main feat of the AstDiff library is analyzing and presenting the differences between two AST subtrees, while this checker only

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 175697. donat.nagy marked 5 inline comments as done. donat.nagy added a comment. Minor simplifications, add tests for macro handling Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.ll

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 15 inline comments as done. donat.nagy added a comment. I will add tests for macro handling later. Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:71 +// Check whether this `if` is part of an `else if`: +if (const auto *IP = +dyn_cast(

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

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 Fil

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. Thank you for this great patch! Did you consider using `clang/Tooling/ASTDiff/*` functionality, as there is the possibility of just comparing the AST 'below' the branches? Please add tests that contain macros as well, and what do you think should happen with them? I

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 6 inline comments as done. donat.nagy added a comment. An example of duplicated code in Clang (it appears in llvm/tools/clang/lib/Frontend/Rewrite/RewriteMacros.cpp starting from line 128): // If this is a #warning directive or #pragma mark (GNU extensions), // comment the

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 174943. donat.nagy added a comment. Implement suggested improvements Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54757 Files: clang-tidy/bugprone/BranchCloneCheck.cpp clang-tidy/bugprone/BranchCloneCheck.h clang-tidy/bugprone/Bu

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

whisperity added a comment. In https://reviews.llvm.org/D54757#1305306, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D54757#1305114, @whisperity wrote: > > > Bar the previous comments, I really like this. The test suite is massive > > and well-constructed. Do we know of any real-world fi

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Eugene.Zelenko added a comment. In https://reviews.llvm.org/D54757#1305114, @whisperity wrote: > Bar the previous comments, I really like this. The test suite is massive and > well-constructed. Do we know of any real-world findings, maybe even from LLVM? GCC 7 introduced -Wduplicated-branches,

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

whisperity added a comment. Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54757 __

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

lebedev.ri added inline comments. Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:43 + + for (unsigned i = 0; i < LHS.size(); i++) { +if (!areStatementsIdentical(LHS[i]->stripLabelLikeStatements(), Eugene.Zelenko wrote: > Please use size_t. Also ``` for

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

Eugene.Zelenko added inline comments. Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:19 + +namespace { + Please use anonymous namespaces only for type declarations. See LLV coding style guide. Comment at: clang-tidy/bugprone/BranchCloneC

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy created this revision. donat.nagy added reviewers: alexfh, hokein, aaron.ballman, xazax.hun, whisperity. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs, mgorny. Implement a check for detecting if/else if/else chains where two or more branches are Type I