Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Febbe (Fabian Keßler)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 1 2022, 4:35 AM (96 w, 3 d)

Recent Activity

Jun 16 2023

Febbe added a comment to D123228: [libc++][WIP] Implement P0881R7 (std::stacktrace).

You could take a look at libbacktrace. The GNU stdlibc++ is using it to generate stacktraces.

Jun 16 2023, 8:12 AM · Restricted Project, Restricted Project

Jun 15 2023

Febbe added a comment to D123228: [libc++][WIP] Implement P0881R7 (std::stacktrace).

What prevents this from a merge? I don't think, that you must implement / support all binary formats to merge this. For now, ELF + DWARF should be enough, right?

Jun 15 2023, 5:34 AM · Restricted Project, Restricted Project

Apr 19 2023

Febbe added a comment to D143870: [clang-format] Remove all include duplicates not only those in the same block.

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 DeduplicateIncludeDirectives: {DID_InSameBlock (default), DID_Never, DID_Allways, DID_OnlyConsecutiveBlocks}

Apr 19 2023, 6:12 AM · Restricted Project, Restricted Project, Restricted Project

Apr 17 2023

Febbe added a comment to D143870: [clang-format] Remove all include duplicates not only those in the same block.

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"
#include "MyDataStructThatContainsPlaformSpecificNamesUsingARCH.h"
#define ARCH "win64"
#include "MyDataStructThatContainsPlaformSpecificNamesUsingARCH.h"

Its not nice, but just because I include it twice doesn't mean its wrong? This change would break code written that way, No?

Apr 17 2023, 2:32 AM · Restricted Project, Restricted Project, Restricted Project

Mar 27 2023

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

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 was specified or be behind an option. We have lots of lambda captures which could be moved with c++14 and having those warnings would lead to lots of suppressions within the code since we only target c++11.

Mar 27 2023, 10:55 AM · Restricted Project, Restricted Project
Febbe added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

@Febbe - checking in. is this still on your radar? I would love to see this merged!

Mar 27 2023, 10:45 AM · Restricted Project, Restricted Project

Feb 16 2023

Febbe added inline comments to D143691: Fix clang-formats IncludeCategory to match the documentation.
Feb 16 2023, 2:23 PM · Restricted Project, Restricted Project, Restricted Project
Febbe added inline comments to D143691: Fix clang-formats IncludeCategory to match the documentation.
Feb 16 2023, 7:07 AM · Restricted Project, Restricted Project, Restricted Project

Feb 15 2023

Febbe added inline comments to D143691: Fix clang-formats IncludeCategory to match the documentation.
Feb 15 2023, 9:22 AM · Restricted Project, Restricted Project, Restricted Project
Febbe updated the diff for D143691: Fix clang-formats IncludeCategory to match the documentation.

Added requested tests:

Feb 15 2023, 9:07 AM · Restricted Project, Restricted Project, Restricted Project
Febbe updated the diff for D143691: Fix clang-formats IncludeCategory to match the documentation.
  • Removed some, not required changes.
  • Reverted mapOptional -> mapRequired change, since it might break existing configs
Feb 15 2023, 8:16 AM · Restricted Project, Restricted Project, Restricted Project

Feb 13 2023

Febbe added a comment to D143870: [clang-format] Remove all include duplicates not only those in the same block.

@HazardyKnusperkeks thank you for the review, I would add another option, but I don't know a good name. I would propose a

Feb 13 2023, 1:20 PM · Restricted Project, Restricted Project, Restricted Project
Febbe added a comment to D143691: Fix clang-formats IncludeCategory to match the documentation.

I've added some elaborations and justifications for the criticized changes.

Feb 13 2023, 1:07 PM · Restricted Project, Restricted Project, Restricted Project

Feb 12 2023

Febbe added reviewers for D143870: [clang-format] Remove all include duplicates not only those in the same block: owenpan, MyDeveloperDay, rymiel, HazardyKnusperkeks.
Feb 12 2023, 5:17 PM · Restricted Project, Restricted Project, Restricted Project
Febbe added a project to D143870: [clang-format] Remove all include duplicates not only those in the same block: Restricted Project.
Feb 12 2023, 5:15 PM · Restricted Project, Restricted Project, Restricted Project
Febbe requested review of D143870: [clang-format] Remove all include duplicates not only those in the same block.
Feb 12 2023, 5:14 PM · Restricted Project, Restricted Project, Restricted Project

Feb 10 2023

Febbe added a comment to D143691: Fix clang-formats IncludeCategory to match the documentation.

I think I am done, and you can review it now.

Feb 10 2023, 6:38 PM · Restricted Project, Restricted Project, Restricted Project
Febbe updated the diff for D143691: Fix clang-formats IncludeCategory to match the documentation.

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 Primary.SecondaryPriority instead of:

Feb 10 2023, 6:31 PM · Restricted Project, Restricted Project, Restricted Project
Febbe updated the diff for D143691: Fix clang-formats IncludeCategory to match the documentation.

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 IncludeCategories.SortPriority to zero with the value from IncludeCategories.Priority
    • the previous approach to set the SortPriority when it's 0 to Priority, instead of initializing it directly as intended had several drawbacks:
      • values of 0 were not possible and resulted in weird behav.
      • when a main include was matched by another matcher, it got annother sortpriority than 0.
Feb 10 2023, 5:58 PM · Restricted Project, Restricted Project, Restricted Project

Feb 9 2023

Febbe updated subscribers of D143691: Fix clang-formats IncludeCategory to match the documentation.
Feb 9 2023, 4:59 PM · Restricted Project, Restricted Project, Restricted Project
Febbe edited projects for D143691: Fix clang-formats IncludeCategory to match the documentation, added: Restricted Project; removed Restricted Project.
Feb 9 2023, 4:58 PM · Restricted Project, Restricted Project, Restricted Project
Febbe added a reviewer for D143691: Fix clang-formats IncludeCategory to match the documentation: Restricted Project.
Feb 9 2023, 4:58 PM · Restricted Project, Restricted Project, Restricted Project
Febbe added inline comments to D143691: Fix clang-formats IncludeCategory to match the documentation.
Feb 9 2023, 4:55 PM · Restricted Project, Restricted Project, Restricted Project
Febbe added a comment to D143691: Fix clang-formats IncludeCategory to match the documentation.

Still some issues with SortPriorities.
The following settings (note the SortPriority of '^<.*' == 1) which will produce an extra group for the attached includes:

Feb 9 2023, 4:49 PM · Restricted Project, Restricted Project, Restricted Project
Febbe requested review of D143691: Fix clang-formats IncludeCategory to match the documentation.
Feb 9 2023, 4:24 PM · Restricted Project, Restricted Project, Restricted Project

Jan 15 2023

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.

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

Jan 11 2023

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

Jan 7 2023

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!

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

Jan 2 2023

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,

Jan 2 2023, 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