Page MenuHomePhabricator

Febbe (Fabian Keßler)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 1 2022, 4:35 AM (51 w, 6 d)

Recent Activity

Sun, Jan 15

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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:

if (SmartPtr new_ptr = steal_ptr(std::move(old_ptr))) {
  ...
}
else {
  return old_ptr; // Now it's moved from and invalid
}

Also this can be happen for similar boolean conditionals in while, for, do while loops.

Interestingly, the logic in bugprone-use-after-move flags this error so maybe we can reuse some of that logic to detect bad std::move.

Sun, Jan 15, 4:46 AM · Restricted Project, Restricted Project

Wed, Jan 11

Febbe added inline comments to D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result.
Wed, Jan 11, 7:46 PM · Restricted Project, Restricted Project

Sat, Jan 7

Febbe added a comment to D141058: [clang-tidy] fix wrong fixup for bugprone-implicit-widening-of-multiplication-result.

Sorry, I don't know how to add tests for such fixes. Could anyone please give me some hints? Thanks!

Sat, Jan 7, 5:14 AM · Restricted Project, Restricted Project

Mon, Jan 2

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

@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 published) and thought it'd be worth sharing notes. I recently turned my non-clang-tidy implementation into a clang-tidy one here. A couple suggestions based on my experience writing a similar tool,

Mon, Jan 2, 10:40 AM · Restricted Project, Restricted Project

Nov 28 2022

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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 also agree that east consts are more readable and fool-proof. it's unfortunate that most of the C++ world is stuck with the west consts.

Nov 28 2022, 2:25 PM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Applied clang-format QualifierAlignment: Left

Nov 28 2022, 1:07 PM · Restricted Project, Restricted Project

Nov 25 2022

Febbe added inline comments to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Nov 25 2022, 10:12 AM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Fixed typo

Nov 25 2022, 10:11 AM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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 explicit guidelines about it in LLVM coding style, but rest of the codebase uses west const convention. so i am not sure if straying away from it here will make much sense.

Nov 25 2022, 9:00 AM · Restricted Project, Restricted Project

Nov 24 2022

Febbe added inline comments to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Nov 24 2022, 2:29 PM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Replaces match clauses with RecursiveASTVisistors

  • doubled performance
  • fixed also a bug in template spezialisations
Nov 24 2022, 2:25 PM · Restricted Project, Restricted Project

Nov 20 2022

Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Improved fixit suggestion.

  • No duplicated replacements.

Removed replacements for macros, since they could be wrong somehow.
Made some helperfunction non-trailing

Nov 20 2022, 9:45 AM · Restricted Project, Restricted Project
Febbe added inline comments to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Nov 20 2022, 7:01 AM · Restricted Project, Restricted Project

Nov 19 2022

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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 rvalue. (In this case, we have a py::object which inherits from py::handle, and is just py::handle with ownership semantics. This allows implicit conversion to a reference at anytime, and therefore std::move has no effect here except to complicate the code a bit.

Nov 19 2022, 11:42 AM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Here's another false positive:

#include <string>

void f(const std::string&);

int main() {
    std::string s;
    f(s.empty() ? "<>" : s);
}
input.cpp:7:26: warning: Parameter 's' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
    f(s.empty() ? "<>" : s);
                         ^
                         std::move( )
Nov 19 2022, 10:41 AM · Restricted Project, Restricted Project

Nov 18 2022

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

The crash is gone.

The false positive with the [m] capture is still present with -std=c++11.

Nov 18 2022, 8:44 AM · Restricted Project, Restricted Project

Nov 15 2022

Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

fixed lamda detection

Nov 15 2022, 2:45 PM · Restricted Project, Restricted Project

Nov 14 2022

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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 location of the capture list for each lambda and check, if the declRefExpr is part of it... . This is bug prone, and possibly slow.

Nov 14 2022, 4:36 PM · Restricted Project, Restricted Project

Nov 13 2022

Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Fixed some false positives:

  • no move for no automatic storage duration
  • no move for lambda captures
Nov 13 2022, 4:58 PM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Getting this false positive:

#include <string>

extern std::string str()
{
	std::string ret;
	return ret.empty() ? ret : ret.substr(1);
}
input.cpp:6:23: warning: Parameter 'ret' is copied on last use, consider moving it instead. [performance-unnecessary-copy-on-last-use]
        return ret.empty() ? ret : ret.substr(1);
                             ^
                             std::move( )

Appears to be caused by the ternary since I also have it inline with a function parameter in another case.

Nov 13 2022, 2:30 AM · Restricted Project, Restricted Project

Nov 9 2022

Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Fixed segfaults due to asserts which were wrongly assumed to be always true

Nov 9 2022, 6:46 AM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Okay, now I am getting what I believe to be segfaults:

#0 0x0000564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
#1 0x0000564383480464 SignalHandler(int) Signals.cpp:0:0
#2 0x00007f7c275c9420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
#3 0x00005643804b0ea5 clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) (.cold) UnnecessaryCopyOnLastUseCheck.cpp:0:0
#4 0x000056438262bba1 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) ASTMatchFinder.cpp:0:0
#5 0x00005643826818df clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) (/home/aaron/git/llvm-project/build/bin/clang-tidy+0x2d258df)

It worked before this PR, now it just crashes on a lot of real world codebases including just trying to run it for a few files on LLVM's own codebase.

Nov 9 2022, 6:39 AM · Restricted Project, Restricted Project

Nov 7 2022

Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Added some tests. Code cleanup.

Nov 7 2022, 4:01 PM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Fixed lValueReference detection in matcher

Nov 7 2022, 1:31 PM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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 arg could be undesirable. For instance take a function that takes in a reference to a String, assigns a new value to the arg reference, and then returns the value of the reference. It suggests calling std::move() on the arg ref when it is returned, which invalidate the reference to the argument.

Nov 7 2022, 12:26 PM · Restricted Project, Restricted Project

Nov 6 2022

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

So I finally had time to apply your feedback.

Nov 6 2022, 12:51 PM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Applied feedback

  • replaced some auto types with the actual type
  • added IncludeStyle to the options list in the documentation
  • Added "Limitations" paragraph, describing known limitations
Nov 6 2022, 12:50 PM · Restricted Project, Restricted Project

Nov 2 2022

Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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?~

Nov 2 2022, 12:20 PM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Applied feedback.

Nov 2 2022, 12:08 PM · Restricted Project, Restricted Project
Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Applied feedback,
Also added the documentation for the second option `BlockedFunctions`

Nov 2 2022, 10:59 AM · Restricted Project, Restricted Project
Febbe added inline comments to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Nov 2 2022, 10:54 AM · Restricted Project, Restricted Project

Nov 1 2022

Febbe updated the diff for D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Added documentation

Nov 1 2022, 5:01 PM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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.

Nov 1 2022, 4:14 PM · Restricted Project, Restricted Project
Febbe retitled D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check from [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 use. - Added BlockLists for false positive types and functions to [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Nov 1 2022, 4:07 PM · Restricted Project, Restricted Project
Febbe requested review of D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Nov 1 2022, 4:02 PM · Restricted Project, Restricted Project

Feb 26 2022

Febbe added a comment to D119481: run-clang-tidy: Fix infinite loop on windows.

@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.

Feb 26 2022, 6:31 AM · Restricted Project, Restricted Project
Febbe added a comment to D119481: run-clang-tidy: Fix infinite loop on windows.

@JonasToth yes, it would be nice, to test this and then push it for me. Also a backport to 14.0 would be good :).

Feb 26 2022, 6:30 AM · Restricted Project, Restricted Project
Febbe updated the diff for D119481: run-clang-tidy: Fix infinite loop on windows.

Applied the feedback

Feb 26 2022, 6:29 AM · Restricted Project, Restricted Project

Feb 10 2022

Febbe added a comment to D119481: run-clang-tidy: Fix infinite loop on windows.

I don't think I have commit rights, so someone of you also have to commit this.

Feb 10 2022, 3:54 PM · Restricted Project, Restricted Project
Febbe added a comment to D119481: run-clang-tidy: Fix infinite loop on windows.

Currently, only tested on Windows.
This should also increase performance, since the path is canonicalized only once.

Feb 10 2022, 2:04 PM · Restricted Project, Restricted Project
Febbe retitled D119481: run-clang-tidy: Fix infinite loop on windows from 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 to run-clang-tidy: Fix infinite loop on windows.
Feb 10 2022, 1:59 PM · Restricted Project, Restricted Project
Febbe requested review of D119481: run-clang-tidy: Fix infinite loop on windows.
Feb 10 2022, 1:58 PM · Restricted Project, Restricted Project

Feb 9 2022

Febbe added a comment to D118847: Added early exit for defaulted FunctionDecls..

Thank you for the review. I am done and you can commit the patch :) . I don't have the rights to commit.

Feb 9 2022, 10:14 AM · Restricted Project, Restricted Project
Febbe updated the diff for D118847: Added early exit for defaulted FunctionDecls..

Last batch of suggested changes

Feb 9 2022, 10:12 AM · Restricted Project, Restricted Project
Febbe added a comment to D118847: Added early exit for defaulted FunctionDecls..

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.

Feb 9 2022, 4:27 AM · Restricted Project, Restricted Project
Febbe added inline comments to D118847: Added early exit for defaulted FunctionDecls..
Feb 9 2022, 4:18 AM · Restricted Project, Restricted Project
Febbe updated the diff for D118847: Added early exit for defaulted FunctionDecls..

Applied feedback

Feb 9 2022, 3:58 AM · Restricted Project, Restricted Project

Feb 8 2022

Febbe added a reviewer for D118847: Added early exit for defaulted FunctionDecls.: kbobyrev.
Feb 8 2022, 7:24 AM · Restricted Project, Restricted Project

Feb 7 2022

Febbe added a comment to D118847: Added early exit for defaulted FunctionDecls..

Push, would be nice to see this in the llvm-14 release. I can also do a review for someone else as a favor.

Feb 7 2022, 8:28 AM · Restricted Project, Restricted Project

Feb 2 2022

Febbe abandoned D118734: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes #53355.

Updated in https://reviews.llvm.org/D118847

Feb 2 2022, 2:06 PM · Restricted Project
Febbe requested review of D118847: Added early exit for defaulted FunctionDecls..
Feb 2 2022, 1:56 PM · Restricted Project, Restricted Project

Feb 1 2022

Febbe requested review of D118734: Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes #53355.
Feb 1 2022, 1:17 PM · Restricted Project