Implement 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.
Details
- Reviewers
alexfh hokein aaron.ballman JonasToth donat.nagy - Commits
- rZORGf4e67f3a2e67: [clang-tidy] new check: bugprone-branch-clone
rZORG48b73106b18d: [clang-tidy] new check: bugprone-branch-clone
rGf4e67f3a2e67: [clang-tidy] new check: bugprone-branch-clone
rG48b73106b18d: [clang-tidy] new check: bugprone-branch-clone
rG7f7dd0900130: [clang-tidy] new check: bugprone-branch-clone
rL360779: [clang-tidy] new check: bugprone-branch-clone
rCTE360779: [clang-tidy] new check: bugprone-branch-clone
rGcd207f155206: [clang-tidy] new check: bugprone-branch-clone
rL348343: [clang-tidy] new check: bugprone-branch-clone
rCTE348343: [clang-tidy] new check: bugprone-branch-clone
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 25441 Build 25440: arc lint + arc unit
Event Timeline
clang-tidy/bugprone/BranchCloneCheck.cpp | ||
---|---|---|
20 | Please use anonymous namespaces only for type declarations. See LLV coding style guide. | |
44 | Please use size_t. | |
123 | Please use size_t. Same for loop variables. Clang complains about such conversions. | |
docs/ReleaseNotes.rst | ||
70 | Please use alphabetical order for new checks list. | |
docs/clang-tidy/checks/bugprone-branch-clone.rst | ||
7 | Please first statement same as Release Notes. |
clang-tidy/bugprone/BranchCloneCheck.cpp | ||
---|---|---|
44 | Also for (unsigned i = 0, unsigned Size = LHS.size(); i < Size; i++) { |
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, so will be good idea to run this check on associated regression(s).
Compared to Donát's checker, GCC's warning seems much more underperforming. See https://godbolt.org/z/Iq3FC9.
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 line out. if (RawTokens[CurRawTok].is(tok::identifier)) { const IdentifierInfo *II = RawTokens[CurRawTok].getIdentifierInfo(); if (II->getName() == "warning") { // Comment out #warning. RB.InsertTextAfter(SM.getFileOffset(RawTok.getLocation()), "//"); // THIS IS... } else if (II->getName() == "pragma" && RawTokens[CurRawTok+1].is(tok::identifier) && (RawTokens[CurRawTok+1].getIdentifierInfo()->getName() == "mark")) { // Comment out #pragma mark. RB.InsertTextAfter(SM.getFileOffset(RawTok.getLocation()), "//"); // IDENTICAL TO THIS } }
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 think it would be very valuable to figure out that different macro expansion result in the same code.
clang-tidy/bugprone/BranchCloneCheck.cpp | ||
---|---|---|
32 | maybe plural for the typename would fit better, as its a vector of multiple elements? | |
47 | You can ellide the single-stmt braces here, maybe its better to move the comment above the for then? | |
72 | This if should always be true, no? The matcher will always bind ifParent | |
84 | This detection can and should land in the matcher, to filter as much as possible already while matching. | |
89 | Please don't use else after return. You can ellide the braces in the if as well. | |
114 | you can ellide the braces. | |
117 | This note is misplaced? The exit is at the break. | |
121 | please dont use const for values, only pointers and references | |
125 | braces, i will stop pointing at it. Please do it on the other parts as well | |
154 | i think you can use early return here, if yes, no else afterwards. | |
165 | typo, branhces | |
188 | i think the name FamilyEnd is not very descriptive, maybe SubsequentBranch or so? | |
197–198 | please use std::distance instead | |
docs/clang-tidy/checks/list.rst | ||
13 | spurious change | |
test/clang-tidy/bugprone-branch-clone.cpp | ||
5 | could you please add tests for ternary operators? |
I will add tests for macro handling later.
clang-tidy/bugprone/BranchCloneCheck.cpp | ||
---|---|---|
72 | I moved this check to the AST matcher. | |
117 | The note was placed correctly, but I replaced it with a break. | |
docs/clang-tidy/checks/list.rst | ||
13 | It was added automatically by the python script, which sorts this list alphabetically. If I remove it now, it will be automatically re-added later. | |
test/clang-tidy/bugprone-branch-clone.cpp | ||
5 | Currently the check does not handle ternary operators, but I added some checks reflecting this. |
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 needs a simple yes/no answer for "Are these branches identical?".
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 some tests to highlight this behavior. I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.
clang-tidy/bugprone/BranchCloneCheck.cpp | ||
---|---|---|
32 | This type represents one branch in a switch statement (which consists of multiple statements). I cannot think of a descriptive, short name which also happens to be plural and I do not want to use a monstrosity like StatementsInSwitchBranch. By the way, can anyone think of shorter, but still clear names for areStatementsIdentical() and areSwitchBranchesIdentical() ? | |
188 | I have renamed FamilyBegin/FamilyEnd to BeginCurrent/EndCurrent. I kept begin and end in the names to emphasize that this is is an iterator range (and altered the comment to include the expression "iterator range"). Is this naming choice clear enough? | |
test/clang-tidy/bugprone-branch-clone.cpp | ||
200 | The tests for macro handling start here. |
The only option I see to detect macro related errors is to use a Lexer, and compare the tokens one by one, but that might be overkill, and I'm aware of a check in the works that already implements that. I think it's perfectly fine not to cover those cases, but you should definitely document it somewhere, preferably in the rst file.
I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.
How about this?
#define PANDA_CUTE_FACTOR 9001 #define CAT_CUTE_FACTOR 9001 if (whatever()) return PANDA_CUTE_FACTOR; else return CAT_CUTE_FACTOR;
Your check would warn here, but it is possible, if not likely that the definition of those macros will eventually change. Again, it's fine not to cover this, but this is a false positive in a sense.
I have to agree with the folks before me, this patch is amazing, great job!
test/clang-tidy/bugprone-branch-clone.cpp | ||
---|---|---|
200 | Maybe add dividers? :) The //===--------------===// thing, you know. |
Agreed with the documentation note.
Starting around with the Lexer here will not help, it is too fragile, as macros can change between different builds and so on.
I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.
How about this?
#define PANDA_CUTE_FACTOR 9001 #define CAT_CUTE_FACTOR 9001 if (whatever()) return PANDA_CUTE_FACTOR; else return CAT_CUTE_FACTOR;Your check would warn here, but it is possible, if not likely that the definition of those macros will eventually change. Again, it's fine not to cover this, but this is a false positive in a sense.
Yes, and it is potentially even build-configuration dependent. This is one of the big issues with macros that are impossible(?) to overcome. I would accept this case in the check. (Making it clear in the doc would not hurt).
docs/clang-tidy/checks/list.rst | ||
---|---|---|
13 | yes. please remove it still, the python script is only run in add_new_check.py, i will commit the fix isolated to master. | |
test/clang-tidy/bugprone-branch-clone.cpp | ||
5 | Thank you. Could you please add a short note to the user-facing documentation that these are not handled. |
clang-tidy/bugprone/BranchCloneCheck.cpp | ||
---|---|---|
32 | The name is ok, I missunderstood the sense of the type. |
test/clang-tidy/bugprone-branch-clone.cpp | ||
---|---|---|
5 | In fact, I could easily extend the functionality of the checker to cover ternary operators. Would it be an useful addition? |
test/clang-tidy/bugprone-branch-clone.cpp | ||
---|---|---|
5 | Sure, it'll be great extension of functionality. |
At the end of the documentation I stated that the check examines the code after macro expansion. Is this note clear enough?
test/clang-tidy/bugprone-branch-clone.cpp | ||
---|---|---|
5 | I added support for ternary operators and documented this fact. |
LGTM, could you please run this check over real world code and check that there are no obvious false positives?
docs/clang-tidy/checks/bugprone-branch-clone.rst | ||
---|---|---|
11 | please do not indent the .. code-block:: c++ but just the code itself, (following too) |
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 together). I didn't go through the whole list, but after examining a few dozen random reports it seems that most of these are true positives: relatively short branches (usually one or two lines of code) are repeated for no obvious reason. I would guess that most of these fall into the "correct but too verbose" case, because otherwise the tests would have caught the problem, but I didn't try to understand their context.
I have seen a few false positives related to preprocessor trickery: when an .inc file is included to create a huge switch, sometimes it will become a huge switch with lots of identical branches. There were also some situations where the check reported identical branches which are annotated with different comments; I don't know if this should be considered a false positive.
If this is acceptable, then I would be grateful if someone would commit this patch for me as I don't have commit rights.
Thank you!
Are the cases with .inc files easily silenceable with // NOLINT comments? In general code-generation through macros is not easily handled with clang-tidy, so all checks have the problem (e.g. GoogleTest is extremly chatty with everything), but would be interesting to know :)
All other cases sound like "a human should look and evaluate" and that's ok for clang-tidy as it doesn't claim finding ONLY bugs, but all kind of issues.
Thank you for the patch, if there are no objections by other reviewers I will commit tomorrow( ~24 hours from now)
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) it should be relatively easy to filter them by location (ignore all reports coming from .inc files).
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 fragments.
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 fragments.
Yes, I think so. The filtering mechanism we have right now are not sufficient i think.
Features similar to pylint would be great, were you can disable certain checks for scopes (functions, modules, classes). With this kind of functionality we could bring clang-tidy to system-level libraries too, as the "unsafe" code is usually chatty in many regards, but with fine-grained and powerful filtering it should be easier to manage the output.
TL;DR I am in favor of such approaches, a short proposal (with what you plan) on the mailing-list wouldn't hurt for more opinions :)
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 locally though.
I'll take a look at this.
The logs are gone -- do you mind me commiting this again to retrieve the assertation?
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()' failed.
$ 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::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, DiagnosticIDs::Level): Assertion `Loc.isValid()' failed. #0 0x00007f263c30ee79 llvm::sys::PrintStackTrace(llvm::raw_ostream&) debugBuild/../llvm/lib/Support/Unix/Signals.inc:494:11 #1 0x00007f263c30f029 PrintStackTraceSignalHandler(void*) debugBuild/../llvm/lib/Support/Unix/Signals.inc:558:1 #2 0x00007f263c30d936 llvm::sys::RunSignalHandlers() debugBuild/../llvm/lib/Support/Signals.cpp:67:5 #3 0x00007f263c30f6db SignalHandler(int) debugBuild/../llvm/lib/Support/Unix/Signals.inc:357:1 #4 0x00007f263bd64890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890) #5 0x00007f2637aefe97 gsignal /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0 #6 0x00007f2637af1801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0 #7 0x00007f2637ae139a __assert_fail_base /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:89:0 #8 0x00007f2637ae1412 (/lib/x86_64-linux-gnu/libc.so.6+0x30412) #9 0x00007f263a64c623 clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) debugBuild/../extra/clang-tidy/Cl angTidyDiagnosticConsumer.cpp:201:17 #10 0x00007f263a647baf clang::tidy::ClangTidyCheck::diag(clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) debugBuild/../extra/clang-tidy/ClangTidyCheck.cpp:23 :3 #11 0x00007f263a28671e clang::tidy::bugprone::BranchCloneCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) debugBuild/../extra/clang-tidy/bugprone/BranchCloneCheck.cpp:200:9 #12 0x00007f263a647be9 clang::tidy::ClangTidyCheck::run(clang::ast_matchers::MatchFinder::MatchResult const&) debugBuild/../extra/clang-tidy/ClangTidyCheck.cpp:31:1 #13 0x00007f263ae60174 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) debugBuild/../clang /lib/ASTMatchers/ASTMatchFinder.cpp:742:7 #14 0x00007f263af4e699 clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) debugBuild/../clang/lib/ ASTMatchers/ASTMatchersInternal.cpp:75:5 #15 0x00007f263ae5fe66 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchWithFilter(clang::ast_type_traits::DynTypedNode const&) debugBuild/../clang/lib /ASTMatchers/ASTMatchFinder.cpp:574:7 #16 0x00007f263ae601c5 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchDispatch(clang::Stmt const*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder .cpp:597:5 #17 0x00007f263ae5fa7d void clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::match<clang::Stmt>(clang::Stmt const&) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:499:3 #18 0x00007f263ae6dedd clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:868:48 #19 0x00007f263ae904b5 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseCompoundStmt(clang::CompoundStmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:2175:1 #20 0x00007f263ae806a2 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::dataTraverseNode(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/tools/clang/include/clang/AST/StmtNodes.inc:73:1 #21 0x00007f263ae7c95e clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:655:7 #22 0x00007f263ae6def9 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:868:3 #23 0x00007f263aeb71ca clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseFunctionHelper(clang::FunctionDecl*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:2020:5 #24 0x00007f263ae677be clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseFunctionDecl(clang::FunctionDecl*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:2025:1 #25 0x00007f263ae61df3 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDecl(clang::Decl*) debugBuild/tools/clang/include/clang/AST/DeclNodes.inc:389:1 #26 0x00007f263ae61281 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:860:3 #27 0x00007f263ae6dc58 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDeclContextHelper(clang::DeclContext*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:1387:7 #28 0x00007f263ae6a3a8 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseTranslationUnitDecl(clang::TranslationUnitDecl*) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:1479:1 #29 0x00007f263ae623f6 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDecl(clang::Decl*) debugBuild/tools/clang/include/clang/AST/DeclNodes.inc:571:1 #30 0x00007f263ae61281 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:860:3 #31 0x00007f263adf7e73 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseAST(clang::ASTContext&) debugBuild/../clang/include/clang/AST/RecursiveASTVisitor.h:183:11 #32 0x00007f263adf7c8d clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:1037:11 #33 0x00007f263ae60d05 clang::ast_matchers::internal::(anonymous namespace)::MatchASTConsumer::HandleTranslationUnit(clang::ASTContext&) debugBuild/../clang/lib/ASTMatchers/ASTMatchFinder.cpp:930:3 #34 0x00007f263608220b clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) debugBuild/../clang/lib/Frontend/MultiplexConsumer.cpp:291:23 #35 0x00007f263253655e clang::ParseAST(clang::Sema&, bool, bool) debugBuild/../clang/lib/Parse/ParseAST.cpp:178:12 #36 0x00007f26360432c2 clang::ASTFrontendAction::ExecuteAction() debugBuild/../clang/lib/Frontend/FrontendAction.cpp:1037:1 #37 0x00007f2636042ce0 clang::FrontendAction::Execute() debugBuild/../clang/lib/Frontend/FrontendAction.cpp:938:7 #38 0x00007f2635fbf6ae clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) debugBuild/../clang/lib/Frontend/CompilerInstance.cpp:945:7 #39 0x00007f2638998382 clang::tooling::FrontendActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) debugBuild/../clang/lib/Tooling/Tooling.cpp:369:14 #40 0x00007f263a5f48d1 clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, llvm::StringRef)::ActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) debugBuild/../extra/clang-tidy/ClangTidy.cpp:540:7 #41 0x00007f2638998218 clang::tooling::ToolInvocation::runInvocation(char const*, clang::driver::Compilation*, std::shared_ptr<clang::CompilerInvocation>, std::shared_ptr<clang::PCHContainerOperations>) debugBuild/../clang/lib/Tooling/Tooling.cpp:344:3 #42 0x00007f2638997385 clang::tooling::ToolInvocation::run() debugBuild/../clang/lib/Tooling/Tooling.cpp:329:3 #43 0x00007f2638999577 clang::tooling::ClangTool::run(clang::tooling::ToolAction*) debugBuild/../clang/lib/Tooling/Tooling.cpp:518:11 #44 0x00007f263a5f283a clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, llvm::StringRef) debugBuild/../extra/clang-tidy/ClangTidy.cpp:562:23 #45 0x0000000000243db8 clang::tidy::clangTidyMain(int, char const**) debugBuild/../extra/clang-tidy/tool/ClangTidyMain.cpp:430:7 #46 0x0000000000243342 main debugBuild/../extra/clang-tidy/tool/ClangTidyMain.cpp:491:3 #47 0x00007f2637ad2b97 __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:344:0 #48 0x000000000024302a _start (debugBuild/bin/clang-tidy+0x24302a)
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/clang-tidy/bugprone/BranchCloneCheck.cpp @@ -197,8 +197,19 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) { 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, + + SourceLocation EndLoc = (EndCurrent - 1)->back()->getEndLoc(); + // If the case statement is generated from a macro, it's SourceLocation + // may be invalid, resuling in an assertation failure down the line. + // While not optimal, try the begin location in this case, it's still + // better then nothing. + if (EndLoc.isInvalid()) + EndLoc = (EndCurrent - 1)->back()->getBeginLoc(); + + if (EndLoc.isMacroID()) + EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc); + + diag(Lexer::getLocForEndOfToken(EndLoc, 0, *Result.SourceManager, getLangOpts()), "last of these clones ends here", DiagnosticIDs::Note); }
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...
test/clang-tidy/bugprone-branch-clone.cpp | ||
---|---|---|
349 | ...except the removal of these -- I guess + if (EndLoc.isMacroID()) + EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc); is responsible for this, but I'm not really sure how else can I avoid the assertation failure. |
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.
Please use anonymous namespaces only for type declarations. See LLV coding style guide.