- User Since
- Feb 5 2017, 1:32 PM (228 w, 4 h)
Mar 22 2021
It's been a while and I almost don't remember what this is. But it was a good fix and there is one LGTM.
Can someone please commit this? Alexander Lanin <email@example.com>
Oct 21 2020
Anyway based on the approach above I found two additional regressions. Testing with real code seems to be kind of important ;-)
- Reported column numbers have been changed. Apparently this cannot be checked by expected-warning and expected-note, so I added another FileCheck pass with a few select test cases.
Maybe first look why the columns have changed and whether it's a real issue.
- All warnings are reported first and then all the notes. Apparently this is not checked by expected-warning and expected-note. This will probably also require to be checked by FileCheck directly.
AFAIK other diagnostic tests don't test the in which the diagnostics are emitted. So I wonder whether this is really required.
I think if would be first good to figure out why this changes and then see whether it's a real issue. If it's an issue it needs to be fixed, else the separate patches would be preferred.
Oct 19 2020
I cannot reproduce the problem and there is just not enough info to go on.
Is there any way to get anything in addition? e.g. run the same test with -vv or add some additional print commands?
Maybe it's some access rights problem, or different version of some non obvious dependency or something like that.
Maybe it's as trivial as having to use double quotes for -checks='-*,bugprone-sizeof-container,modernize-use-auto'.
It seems run-clang-tidy exits with code 2, but there is no way it would do that. So I'm totally lost.
Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <firstname.lastname@example.org>
Oct 18 2020
I'm not convinced the new approach is better in all cases, so I'm somewhat concerned about regressions from the current behaviour. Did you test this patch on a codebase to see the difference between the previous and new behaviour?
There are no warnings in llvm or ccache, all I found is a low quality codebase at work which does produce 1007 unique Wdocumentation warnings (see wc command).
Oct 17 2020
Oct 16 2020
Readd removed 'print help without crashing' test
Added one missing expect statement in this revision.
Oct 14 2020
@JonasToth: Cannot add assert. EndLoc is indeed invalid in case BeginLoc is outside the macro and EndLoc would be inside the macro. I've added a comment explaining that.
Oct 12 2020
@JonasToth I fully realize that it's voluntary and I do not have a solution. I have the same problem in a different area. Not sure what shall become of some OSS projects when it takes that long to get a patch in. But that's a different discussion.
Rebased and stored at https://github.com/AlexanderLanin/llvm-project/tree/Wdocumentation-fix-its so I have a branch next time I need to rebase :-)
Rebased and stored at https://github.com/llvm/llvm-project/compare/9d6d4b07a...AlexanderLanin:readability-braces-around-statements?expand=1 so I have a branch next time I need to rebase :-)
Sep 25 2020
Off-Topic: I was just attempting to apply this to my codebase at work.
Seems this will be more challenging than anticipated 😉
My best guess is this is related to D72730
"Minimal" example for the behavior I described before.
Sep 23 2020
Could you please provide me a full reproducer (optimally without dependencies on includes/libraries)?
I can certainly do that based on my old version from ~9 months ago or preferably if you provide me some branch somewhere (github?). I have trouble applying the latest diff to latest master myself.
Sep 3 2020
Jul 30 2020
Addressed findings by @njames93
Jul 29 2020
The reason was that I thought that line doesn't do anything.
Of course "not crashing" is a valid expectation. If nothing else, then "not crashing" should indeed be tested.
I'll try to improve this ancient diff.
Last change was over 3 months ago. Should this be merged or abandoned?
It doesn't solve everything. It doesn't improve everything. But it's a fix to two specific bugs.
Is someone missing as a reviewer to get this accepted/rejected any faster?
Sorry, but I have to ask: Is there no interest in improving clang-tidy?
Don't get me wrong. I know it's mostly volunteered time. That's also why this ping took almost 2 months for me.
I frankly lost interest in contributing as I have trouble getting any patches included - and I don't have that much free time anymore. Obviously the time issue is more important, but the other one is still a problem.
Jun 2 2020
Apr 24 2020
Mailing list was clear about AST having to many users. No change to clang AST.
Will hopefully be resolved by D75813 instead.
Apr 11 2020
- Add test cases exactly as in bug reports.
- Test case from https://bugs.llvm.org/show_bug.cgi?id=43808 triggered a second bug due to the short parameter names used. Removed incorrect quick exit in SimpleTypoCorrector (MinPossibleEditDistance).
- The two requested cases don't work since 'aaab' is as close to 'aaa' as it is to 'aab' (one char). Nothing is changed as there is no best match available. Instead two new test cases with aaab, aaac and aaad added (test3to3_allAlmost + test3to3_allAlmost_rev).
Apr 10 2020
I need to have a look why the mentioned test case is failing. I'll also add the two examples you provided to the next upload.
Apr 1 2020
I have an opinion on NullStmt, but I would rather see this merged ;-)
Behavior is same as before.
Mar 23 2020
More tests added to prevent further regressions. It's quite difficult to figure out tests which might fail, but now there is a whole bunch of them so it looks good.
Added explicit references from tests to the two bug reports.
Mar 21 2020
Thanks for the feedback!!
Mar 19 2020
Just new tests, easy to review :-)
new test case as requested (line 256)
Mar 18 2020
Add further tests from more bug reports.
Mar 17 2020
Mar 16 2020
Accidentally reverted test file renaming in last commit. Fixed now.
Rebased on current master.
Mar 14 2020
One more thought: This originated for me from attempting to fix braces-around-statements which has quite some trouble finding the correct place to place the }. However this is a good definition of "end location of statement" isn't it? The place where you can end the scope. The place where you could place the next statement!
Thanks for the feedback. I was trying to start a discussion on cfe-dev where I referenced this commit, but I'm also fine having it here! There you'll also find some rationale. Just worried it will get out of sync.
Mar 12 2020
Fine for me. Fixes newline bug in https://bugs.llvm.org/show_bug.cgi?id=45150.
However I don't have "review privileges" here.
Unfortunately I cannot say whether the code is good, but the fix works and certainly helps readability-braces-around-statements which can sometimes add multiple } at the same offset to close several scopes.
Mar 11 2020
Mar 10 2020
Rebased. Solves the svn issue as it's gone in the meantime.
Mar 9 2020
Mar 7 2020
- Extended Test for another macro scenario.
- Added graceful termination to handle problematic locations.
- Changed Lexer::findNextToken to no longer run into that graceful termination. Test is passing. I'm not sure whether that's a good idea? check-clang and check-clang-tools are passing with this change.
Oh sorry. It's crashing. I'll look into it.
Feb 29 2020
Here is the r_brace use case:
Feb 23 2020
Slightly more code changed, assuming it's better understandable now. Review findings incorporated.
Feb 20 2020
Feb 19 2020
Could someone commit this as I cannot? Thanks.
Alexander Lanin <email@example.com>
Feb 18 2020
Feb 9 2020
This was an accident on github. Can/should be closed/deleted here. Sorry!
The proper submission is at https://reviews.llvm.org/D74299
I tested this in WSL1 with ninja check-clang-tools.
Is there anything else I can do (besides setting up a second OS)?
On second thought maybe this should be fixed in clang-tidy and not here?
Maybe besides -export-fixes there should be an -export-warnings or just -yaml-export?
Feb 3 2020
@aaron.ballman I was ready to complain, but you are right. It would make absolutely no sense to move the const, it's just where it should be.
Feb 2 2020
Feb 1 2020
This is now detected by cppcoreguidelines-macro-usage, so this seems out of date.
Review findings applied (no real measurable difference in speed, maybe 100ms) but it's indeed more readable.
Without this fix the export took 12.96 seconds, with this patch 11.07 seconds.
Difference is even bigger when there are less auto fixable findings (70% with FIX-IT in my example).
Jan 26 2020
Jan 22 2020
Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in misc-misplaced-const.cpp - I really do not understand how that happened.
Rebased and verified with check-clang-tools.
Could someone commit this? As I can not.
Alexander Lanin <firstname.lastname@example.org>
Jan 16 2020
Thanks for the review!
Jan 15 2020
Hi, could anyone provide feedback here?
Windows and RC Versions of git do not work with format-patch.
Jan 14 2020
Important: I'm not a python expert. It works fine as far as I can tell. I would feel better if someone with more than a day python experience would say that it is fine.
Jan 12 2020
Updated revision with llvm_unreachable
Jan 8 2020
I guess it's not so easy to fix as you reported this 1,5 years ago. That's an even better reason to update the documentation to work around the issue.
Jan 7 2020
The only difference is the git version in the end:
looking at this at least many are indeed valid results:
I guess false positives could be reduced by eliminating those cases where the variables are (intentionally) modified.
ping? Could someone push this please.
Jan 3 2020
Getting Started since it's Getting Started with the LLVM System
Jan 2 2020
Not sure we can call this updated proposal 'love': it's definitely not the goal to have less documentation, just less redundancy.
I do not have commit rights to the repository, someone please commit this change.
Alexander Lanin <email@example.com>
@Jim git commit --amend --author="Alexander Lanin <firstname.lastname@example.org>"
Jan 1 2020
Removed change in hacking page as discussed.
Can someone commit this as apparently I cannot do it myself (I'll create a PR with updated getting started documentation later...), thanks.