[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

Febbe updated this revision to Diff 411606. Febbe added a comment. Applied the feedback CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119481/new/ https://reviews.llvm.org/D119481 Files: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py Index:

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

Febbe added a comment. @JonasToth yes, it would be nice, to test this and then push it for me. Also a backport to 14.0 would be good :). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119481/new/ https://reviews.llvm.org/D119481 ___

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

Febbe added a comment. @salman-javed-nz you're welcome, I only fixed it because I saw the bug in the trace directly. Normally, I would only fix C/C++ stuff :D. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119481/new/ https://reviews.llvm.org/D119481

[PATCH] D118734: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes #53355

Febbe created this revision. Herald added a subscriber: carlosgalvezp. Febbe requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D118734 Files:

[PATCH] D118847: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes llvm#53355

Febbe created this revision. Herald added a subscriber: carlosgalvezp. Febbe requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D118847 Files:

[PATCH] D118734: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes #53355

Febbe abandoned this revision. Febbe added a comment. Updated in https://reviews.llvm.org/D118847 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118734/new/ https://reviews.llvm.org/D118734 ___

[PATCH] D118847: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes llvm#53355

Febbe added a comment. Push, would be nice to see this in the llvm-14 release. I can also do a review for someone else as a favor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118847/new/ https://reviews.llvm.org/D118847

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

Febbe updated this revision to Diff 407115. Febbe retitled this revision from "Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes llvm#53355" to "Added early exit for defaulted FunctionDecls.". Febbe edited the summary of this revision.

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

Febbe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp:71 + template + requires(T{0}) // + friend constexpr auto kbobyrev wrote: > nit: `//` looks unrelated I used the `//` to

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

Febbe marked an inline comment as done. Febbe added a comment. Thank you for the review. I am done and you can commit the patch :) . I don't have the rights to commit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118847/new/ https://reviews.llvm.org/D118847

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

Febbe updated this revision to Diff 407200. Febbe marked 4 inline comments as done. Febbe added a comment. Last batch of suggested changes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118847/new/ https://reviews.llvm.org/D118847 Files:

[PATCH] D118847: Added early exit for defaulted FunctionDecls.

Febbe added a comment. Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)? Also, is the commit added automatically to the repo, or do I / another one have to rebase it. Sorry for those questions, this is my first contribution to llvm. CHANGES SINCE LAST

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows find_compilation_database checked only for "/" as exit point, but on windows, this root is impossible. Fixes #53642

Febbe created this revision. Herald added a subscriber: carlosgalvezp. Febbe requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D119481 Files:

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

Febbe added a comment. Currently, only tested on Windows. This should also increase performance, since the path is canonicalized only once. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119481/new/ https://reviews.llvm.org/D119481

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

Febbe added a comment. I don't think I have commit rights, so someone of you also have to commit this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119481/new/ https://reviews.llvm.org/D119481 ___

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check - finds occurences of last uses, which are copied. - suggests adding a `std::move`. - detects assignments as last

Febbe created this revision. Herald added subscribers: carlosgalvezp, xazax.hun. Herald added a project: All. Febbe requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. I also have to add the http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-copy-on-last-use.html page, but I don't have any clue how, and where to start. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 472458. Febbe added a comment. Added documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files: clang-tools-extra/clang-tidy/performance/CMakeLists.txt

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#4018548 , @ccotter wrote: > @Febbe - Really glad to see this phab up! Not having realized this phab was > in progress, I implemented this a few months back (though, originally not as > a clang-tidy tool and never

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

Febbe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp:18 // CHECK-NOTES-C:(ptrdiff_t)( ) // CHECK-NOTES-CXX: static_cast(

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:79 + +static auto findDeclRefBlock(CFG const *TheCFG, DeclRefExpr const *DeclRef) +-> FindDeclRefBlockReturn { njames93 wrote: > We generally

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3936944 , @firewave wrote: > The crash is gone. > > The false positive with the `[m]` capture is still present with `-std=c++11`. Does only a warning appear? Or also a fix? Currently, only a warning should appear, but

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 476757. Febbe marked 3 inline comments as done and 3 inline comments as done. Febbe added a comment. Improved fixit suggestion. - No duplicated replacements. Removed replacements for macros, since they could be wrong somehow. Made some helperfunction

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 478348. Febbe added a comment. Applied clang-format `QualifierAlignment: Left` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files:

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. >> It is also completely irrelevant, because a new programmer will not >> understand that const T const * is actually T const* and not T const * >> const. An experienced programmer can understand it well either way. > > not sure I follow the argument here, but surely I

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 475589. Febbe added a comment. fixed lamda detection Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files: clang-tools-extra/clang-tidy/performance/CMakeLists.txt

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.cpp:215 "-bugprone-use-after-move", + // Using an CFG and might crash on invalid code: +

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3950722 , @kadircet wrote: > another thing that i noticed is usage of east consts in the implementation > files. no one seem to have brought it up so far (at least none that i can > see). > in theory there are no

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 477997. Febbe marked 3 inline comments as done. Febbe added a comment. Fixed typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files:

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 477846. Febbe marked an inline comment as done. Febbe added a comment. Herald added subscribers: kadircet, arphaman. Replaces match clauses with `RecursiveASTVisistor`s - doubled performance - fixed also a bug in template spezialisations Repository: rG

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe marked 2 inline comments as done. Febbe added inline comments. Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:123 + hasLHS(ignoringParenImpCasts(declRefExpr(equalsNode(DeclRef)), + Context); + return

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3936944 , @firewave wrote: > Here's another false positive: > > #include > > void f(const std::string&); > > int main() { > std::string s; > f(s.empty() ? "<>" : s); > } > > > >

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3923643 , @Skylion007 wrote: > The main false positive I also keep seeing is in pybind11 where it suggests > call an std::move() for an implicit conversion, but the target of the > implicit conversion does not have an

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#4047828 , @Skylion007 wrote: > I am trying this in the wild and getting some false positives where it tries > to call std::move inside loop conditions and in the boolean condition for an > if statement. Stuff like: >

[PATCH] D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result

Febbe added a comment. In D141058#4028768 , @v1nh1shungry wrote: > Sorry, I don't know how to add tests for such fixes. Could anyone please give > me some hints? Thanks! It seems like the tests for this check do not contain checks for the fixups

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 473770. Febbe added a comment. Fixed lValueReference detection in matcher Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files:

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3912243 , @Skylion007 wrote: > One other bug I found with this diff, is it seems to suggest calling > std::move() on function args that are references, despite the fact that > invalidating the reference to the input

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 473819. Febbe added a comment. Added some tests. Code cleanup. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files:

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe marked 10 inline comments as done. Febbe added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/performance/unnecessary-copy-on-last-use.rst:6 + +Finds variables of non-trivially-copyable types, that are used in a copy construction on their last

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 472698. Febbe added a comment. Applied feedback, Also added the documentation for the second option ``BlockedFunctions`` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 472722. Febbe added a comment. Applied feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files: clang-tools-extra/clang-tidy/performance/CMakeLists.txt

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe marked 4 inline comments as done. Febbe added a comment. I applied the rest of your feedback. There are other usages of `auto` like `auto FoundUsage` which is a Usage for example. ~Shall I also replace those obvious cases?~ - I just checked the guidelines. Obvious cases can be auto.

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3915526 , @Skylion007 wrote: > Okay, now I am getting what I believe to be segfaults: > > #0 0x564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0 > #1 0x564383480464 SignalHandler(int)

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 474260. Febbe added a comment. Fixed segfaults due to asserts which were wrongly assumed to be always true Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137205/new/ https://reviews.llvm.org/D137205 Files:

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. So I finally had time to apply your feedback. Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:141 + assert(BlockElement.DeclRefBlock); + auto Block = BlockElement.DeclRefBlock->succs(); + // No

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 473508. Febbe marked 19 inline comments as done. Febbe added a comment. Applied feedback - replaced some auto types with the actual type - added `IncludeStyle` to the options list in the documentation - Added "Limitations" paragraph, describing known

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#3920016 , @firewave wrote: > Getting this false positive: > > #include > > extern std::string str() > { > std::string ret; > return ret.empty() ? ret : ret.substr(1); > } > > > > input.cpp:6:23:

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. I have a problem with lambdas capturing by copy implicitly (`[=]()...`). It seems like, that child nodes are not reachable via a MatchFinder unless they are spelled in source. I actually don't know how to prevent a fix elegantly: My proposed idea is, to check the source

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe updated this revision to Diff 475030. Febbe marked 5 inline comments as done. Febbe added a comment. Fixed a lot of false positives: - no move for no automatic storage duration - no move for lambda captures Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe added inline comments. Comment at: clang/unittests/Format/SortIncludesTest.cpp:548 + EXPECT_EQ("#include \"a.h\"\n" +"#include \"llvm/a.h\"\n" +"#include \"b.h\"\n" HazardyKnusperkeks wrote: > While I may accept there are more than

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe added inline comments. Comment at: clang/unittests/Format/SortIncludesTest.cpp:548 + EXPECT_EQ("#include \"a.h\"\n" +"#include \"llvm/a.h\"\n" +"#include \"b.h\"\n" HazardyKnusperkeks wrote: > Febbe wrote: > > HazardyKnusperkeks

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

Febbe added a comment. In D143870#4273075 , @MyDeveloperDay wrote: > I know it might not seem an obvious use case but there really isn't a > requirement to not include header files more than once.. imagine if I have > > #define ARCH "win32" >

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

Febbe added a comment. Actually, I already wanted to add an enum, controlling this behavior. But even with an enum, this check should only remove includes in consecutive blocks, which could be relocated (are not split by `#defines` or a comment) Or I should add the options to

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#4222920 , @ccotter wrote: > @Febbe - checking in. is this still on your radar? I would love to see this > merged! Yes, this is still on my radar, but currently, I am not satisfied with my solution. There are too many

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

Febbe added a comment. In D137205#4224236 , @firewave wrote: > Some additional thoughts I had a while ago about something I have raised > before: > I think the warnings which can only be fixed with c++14 should either only be > issued if that standard

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe updated this revision to Diff 496647. Febbe added a comment. Fixed the Unit Tests - rewritten one test, which made the assumption, that there can be only one main header. - it now asserts, that all matching headers are considered as main header. - replaced initialisations of

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe added a comment. I think I am done, and you can review it now. With the latest changes, SortPriority is no longer required to match Priority, it can always be 0, since Priority is now used in the sorting algorithm. This is what the documentation tends to depict: "and #includes are sorted

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe updated this revision to Diff 496650. Febbe added a comment. Added priority to the sorting algorithm. This prevents unwanted splits and weird resorts of groups with the same priority, but with different and overlapping (other groups) SortPriority. The new system can now be understood as

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe added inline comments. Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:16 #include namespace clang { `` is used, but not included. Currently, it works, since another header already included it. Repository: rG LLVM Github Monorepo

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe added a comment. Still some issues with: SortPriorities The following settings (note the SortPriority of '^<.*' == 1) which will produce an extra group for the attached includes: IncludeCategories: - Regex: '^' Priority:2 SortPriority:2 - Regex:

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe created this revision. Herald added a project: All. Febbe requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. - Main Include Files have now always prio (0,0) - They can't be matched by negative matchers anymore. - `SortPriority` now

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe added a comment. I've added some elaborations and justifications for the criticized changes. Comment at: clang/lib/Format/Format.cpp:1379 LLVMStyle.IncludeStyle.IncludeCategories = { - {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false}, -

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

Febbe added a comment. @HazardyKnusperkeks thank you for the review, I would add another option, but I don't know a good name. I would propose a `boolean` `IncludeDeduplicateInAllBlocks` which defaults to zero. First an `Include`, to keep include-sorting related options together in the

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe marked 2 inline comments as done. Febbe added inline comments. Comment at: clang/lib/Format/Format.cpp:1379 LLVMStyle.IncludeStyle.IncludeCategories = { - {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false}, - {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false}, -

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe updated this revision to Diff 497687. Febbe marked 3 inline comments as done. Febbe added a comment. - Removed some, not required changes. - Reverted mapOptional -> mapRequired change, since it might break existing configs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143691: Fix clang-formats IncludeCategory to match the documentation

Febbe updated this revision to Diff 497701. Febbe added a comment. Added requested tests: - since we now support priorities, equal to 0 (the main include priority) sorting should work now for those. - multiple files can be main-includes now, the tests expects that now. - negative priorities do

[PATCH] D143870: [clang-format] Remove all include duplicates not only those in the same block

Febbe created this revision. Herald added a project: All. Febbe requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This change aims to remove all duplicate include directives, instead of only those, which are in the same block. This change