[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added a comment. In D54737#1314946 , @hwright wrote: > Oh, and thanks for taking the time to review this. :) No problem! I will commit on Monday evening (Europe evening) if there are no further comments from other reviewers. Thank you for the

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

JonasToth added a comment. > Wiring out the lexical comparison and AST based comparison sounds like an > interesting idea, however IMHO such a setting is too coarse-grained on the > file level. My guess would be that it depends from expression (fragment) to > expression fragment how you want

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

JonasToth added a comment. In D55125#1314741 , @Szelethus wrote: > @JonasToth this is the `Lexer` based expression equality check I talked about > in D54757#1311516 . The point of > this patch is that the

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. Only the test and your opinion on the `Optional` thing, If you want to keep it that way its fine. LGTM afterwards :) Comment at:

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. LGTM, could you please run this check over real world code and check that there are no obvious false positives? Comment at: docs/clang-tidy/checks/bugprone-branch-clone.rst:10 + + .. code-block:: c++ + please do not indent the `..

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added inline comments. Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:31 +/// an if/else if/else chain is one statement (which may be a CompoundStmt). +using SwitchBranch = llvm::SmallVector; +} // anonymous namespace donat.nagy wrote: >

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. In D54757#1311516 , @Szelethus wrote: > In D54757#1311468 , @donat.nagy > wrote: > > > **Macros:** > > > > The current implementation of the check only looks at the preprocessed > >

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61 + + if (SimpleArg) { diag(MatchedCall->getBeginLoc(), hwright wrote: > JonasToth wrote: > > hwright wrote: > > > JonasToth wrote: > > > > The diagnostic is not

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

JonasToth added a comment. The issues seems to be resolved for me as well, i will post this patch to the mailing list and ask the other guy if his build is fine, too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html class DurationDivisionCheck : public ClangTidyCheck { hwright wrote: > JonasToth wrote: >

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added a comment. LG from my side, only the style nits left. other reviewers are invited to take a look too :) Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:22 + +// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`), +// return its

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

JonasToth added a comment. Thank you for the fix! I will try this version as soon as I am at home (~2 hours from now) and report back if the issues I had are gone. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationRewriter.h:62 + +AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, + DurationConversionFunction) { JonasToth wrote: > I think you can even make this an

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationRewriter.h:62 + +AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, + DurationConversionFunction) { I think you can even make this an `AST_MATCHER(FunctionDecl,

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50 + static const std::unordered_map> + InverseMap( hwright wrote: > hwright wrote: > > JonasToth wrote: > > > This variable is a little hard to read. Could you make

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth updated this revision to Diff 175459. JonasToth added a comment. - after countless attempts of fixing the unicode problem, it is finally done. - Remove all unnecessary whitespace - remove the `xxx warnings generated.` as well This setup now runs in my BB and is a good approximation

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

JonasToth added subscribers: sammccall, JonasToth. JonasToth added a comment. I like the overview, maybe a link to `clangd` here might help, as there is currently a lot of effort of integrating `clang-tidy` into it. (@sammccall WDYT?) Comment at: docs/clang-tidy/index.rst:23

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

JonasToth added a comment. In D54395#1309423 , @lebedev.ri wrote: > Looks reasonable to me (or at least all these tests make this look good :)), > but i'm not sure i can fully review this. No problem ;) Repository: rCTE Clang Tools Extra CHANGES

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

JonasToth created this revision. JonasToth added reviewers: alexfh, aaron.ballman, hokein, shuaiwang, lebedev.ri. Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai. This patch connects the check for const-correctness with the new general utility to add const to

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

JonasToth updated this revision to Diff 175445. JonasToth added a comment. - Merge branch 'master' into check_const Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45444/new/ https://reviews.llvm.org/D45444 Files:

[PATCH] D52690: [clang-tidy] NFC use CHECK-NOTES in tests for misc-misplaced-const

JonasToth abandoned this revision. JonasToth added a comment. not so relevant and spams my view Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52690/new/ https://reviews.llvm.org/D52690 ___ cfe-commits

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

JonasToth updated this revision to Diff 175444. JonasToth added a comment. reabse to master + ping :) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54395/new/ https://reviews.llvm.org/D54395 Files: clang-tidy/performance/ForRangeCopyCheck.cpp

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:25 +static llvm::Optional getScaleForInverse(llvm::StringRef Name) { + static const std::unordered_map ScaleMap( + {{"ToDoubleHours", DurationScale::Hours}, hwright

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

JonasToth added a comment. 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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth updated this revision to Diff 175063. JonasToth added a comment. Push some fixes i did while working with this script, for reference of others potentially looking at this patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54141 Files:

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

JonasToth updated this revision to Diff 174942. JonasToth added a comment. - Merge branch 'master' into check_const Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45444 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt

[PATCH] D54745: [clang-tidy] Don't generate incorrect fixes for class with deleted copy constructor in smart_ptr check.

JonasToth added a comment. IMHO this patch is fine, but i think a language expert (not me :D) should take a look (@aaron.ballman ?) as its complicated :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54745 ___ cfe-commits mailing

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

JonasToth added inline comments. Comment at: unittests/clang-tidy/AddConstTest.cpp:733 + StringRef T = "template void f(T v) \n"; + StringRef S = "{ T target = v; }"; + auto Cat = [](StringRef S) { return (T + S).str(); }; alexfh wrote: > It would be

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

JonasToth updated this revision to Diff 174828. JonasToth added a comment. add tests for different template instantiations Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54395 Files: clang-tidy/performance/ForRangeCopyCheck.cpp

[PATCH] D54745: [clang-tidy] Don't generate incorrect fixes for class with deleted copy constructor in smart_ptr check.

JonasToth added a comment. Does make_unique require the copy constructor if it could move? And would the same argument apply to the move-constructors as the arguments are forwarded? What would happen in the obscure case of a public copy-constructor, but private move-constructor (not saying it

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added inline comments. Comment at: test/clang-tidy/abseil-duration-comparison.cpp:89 + // CHECK-FIXES: d1 > d2; + + // Check against the LHS JonasToth wrote: > What would happen for a type, that can implicitly convert to a duration or > double/int.

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

JonasToth added a comment. Could you please upload the diff with full context? Especially the parts with refactoring are harder to judge if the code around is not visible. Thank you :) Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:25 +static llvm::Optional

[PATCH] D54262: [clang-tidy] Add `delete this` bugprone check (PR38741)

JonasToth added a comment. Ok, Thank you for looking into it. You can abondon the revision in phabricator (Selection above comment field at the bottom of the page) if you wish. Am 19.11.18 um 23:36 schrieb Mateusz Maćkowski via Phabricator: > m4tx added a comment. > > After thinking about the

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. So, finally LGTM :) Please give @aaron.ballman a chance to comment, as he reviewed too. Thanks for your patch! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

JonasToth added a comment. Only a few nits from me. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:30 + WarnOnFloatingPointNarrowingConversion( + Options.get("WarnOnFloatingPointNarrowingConversion", 0)) {} + I would make

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. Committed r346665. Thank you very much for the patch! Repository: rL LLVM https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

This revision was automatically updated to reflect the committed changes. Closed by commit rL346665: [clang-tidy] new check: bugprone-too-small-loop-variable (authored by JonasToth, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D53974

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

JonasToth updated this revision to Diff 173653. JonasToth added a comment. - Merge branch 'master' into check_const Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45444 Files: clang-tidy/cppcoreguidelines/CMakeLists.txt

[PATCH] D54399: Move ExprMutationAnalyzer to Tooling/Analysis (1/3)

JonasToth added a comment. Will you update the existing clang-tidy checks as well? Repository: rC Clang https://reviews.llvm.org/D54399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

JonasToth created this revision. JonasToth added reviewers: aaron.ballman, hokein, alexfh, shuaiwang, lebedev.ri. Herald added subscribers: cfe-commits, xazax.hun, mgorny. JonasToth added a dependent revision: D45444: [clang-tidy] implement new check for const-correctness. This patch extends the

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added inline comments. Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:138 + bool cond = false; + for (short i = 0; i < (cond ? 0 : size); ++i) { +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. > Yes, that's right, these are not full false positives, but the check's main > focus is on those loops which are runtime dependent. If a loop's upper bound > can be calculated in compile time then this loop should be caught by a > compiler warning based on the

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. >> I would be very interested why these false positives are created. Is there a >> way to avoid them and maybe it makes sense to already add `FIXME` at the >> possible places the check needs improvements. > > voidForLoopFalsePositive() and voidForLoopFalsePositive2()

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

JonasToth added inline comments. Comment at: docs/clang-tidy/checks/readability-redundant-preprocessor.rst:34 + + #ifdef FOO + #ifndef FOO // inner ifndef is considered redundant The indendation for these examples (following as well) is not enough. Please use 2

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. Thank you for checking these two projects! In https://reviews.llvm.org/D53974#1294375, @ztamas wrote: > I have the result after running the current version of the check on > LibreOffice. > > Found around 50 new issues were hidden by macros: > >

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

JonasToth added a comment. > Done. Please also mark the inline comments as done. Otherwise its hard to follow if there are outstanding issues somewhere, especially if code moves around. https://reviews.llvm.org/D54349 ___ cfe-commits mailing list

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth added a comment. In https://reviews.llvm.org/D53771#1294244, @lebedev.ri wrote: > *Maybe* we should be actually diagnosing the each actual declaration, not > just the `typeloc`. > Else if you use one `typedef`, and `// NOLINT` it, you will silence it for > all the decls that use it.

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. Please give other reviewers a chance to take a look at it too. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53771

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth added a comment. from my side mostly ok. I think the typedef points should be clarified, but I dont see more issues. Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:32 + +using k = int[4]; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth added inline comments. Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:26 + const clang::Type *TypeNode = Node.getTypePtr(); + return (TypeNode != nullptr && + InnerMatcher.matches(*TypeNode, Finder, Builder)); Nit: these parens are

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. > I'll run it on LibreOffice code again and we'll see. Perfect, if you have the time, running it against LLVM would be very interesting as well. >> Do you have commit access? > > Commit access? This is my first patch. If you plan to regularly contribute to LLVM you

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-pp'

JonasToth added a comment. Hi vmiklos, thank you for working on this patch! I added a few comments. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:83 "readability-misplaced-array-index"); +CheckFactories.registerCheck("readability-redundant-pp");

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

This revision was automatically updated to reflect the committed changes. Closed by commit rL346555: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds… (authored by JonasToth, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

JonasToth updated this revision to Diff 173425. JonasToth added a comment. - fix comment Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54281 Files: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

[PATCH] D54307: [ASTMatchers] overload ignoringParens for Expr

This revision was automatically updated to reflect the committed changes. Closed by commit rC346554: [ASTMatchers] overload ignoringParens for Expr (authored by JonasToth, committed by ). Changed prior to commit: https://reviews.llvm.org/D54307?vs=173422=173424#toc Repository: rC Clang

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

JonasToth updated this revision to Diff 173423. JonasToth added a comment. - use ignoringParens instead Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54281 Files: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

[PATCH] D54307: [ASTMatchers] overload ignoringParens for Expr

JonasToth updated this revision to Diff 173422. JonasToth added a comment. - add unit test Repository: rC Clang https://reviews.llvm.org/D54307 Files: docs/LibASTMatchersReference.html include/clang/ASTMatchers/ASTMatchers.h lib/ASTMatchers/Dynamic/Registry.cpp

[PATCH] D54307: [clang] overload ignoringParens for Expr

JonasToth added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:814 /// would match the declaration for fp. -AST_MATCHER_P(QualType, ignoringParens, - internal::Matcher, InnerMatcher) { +AST_MATCHER_P_OVERLOAD(QualType, ignoringParens,

[PATCH] D54307: [clang] overload ignoringParens for Expr

JonasToth updated this revision to Diff 173391. JonasToth marked 3 inline comments as done. JonasToth added a comment. - add unit test Repository: rC Clang https://reviews.llvm.org/D54307 Files: docs/LibASTMatchersReference.html include/clang/ASTMatchers/ASTMatchers.h

[PATCH] D54307: [clang] overload ignoringParens for Expr

JonasToth added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:814 /// would match the declaration for fp. -AST_MATCHER_P(QualType, ignoringParens, - internal::Matcher, InnerMatcher) { +AST_MATCHER_P_OVERLOAD(QualType, ignoringParens,

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. Did you run this check in its final form against a bigger project? These results would be interesting. Do you have commit access? Repository: rCTE Clang Tools Extra

[PATCH] D54307: [clang] overload ignoringParens for Expr

JonasToth added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:814 /// would match the declaration for fp. -AST_MATCHER_P(QualType, ignoringParens, - internal::Matcher, InnerMatcher) { +AST_MATCHER_P_OVERLOAD(QualType, ignoringParens,

[PATCH] D54307: [clang] overload ignoringParens for Expr

JonasToth created this revision. JonasToth added a reviewer: aaron.ballman. Herald added a subscriber: cfe-commits. This patch allows fixing PR39583. Repository: rC Clang https://reviews.llvm.org/D54307 Files: docs/LibASTMatchersReference.html include/clang/ASTMatchers/ASTMatchers.h

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

JonasToth updated this revision to Diff 173311. JonasToth added a comment. - use ignoringParens instead Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54281 Files: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:65 + unless(isInsideOfRangeBeginEndStmt()), + unless(hasSourceExpression(ignoringParenCasts(stringLiteral() .bind("cast"),

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

JonasToth created this revision. JonasToth added reviewers: aaron.ballman, alexfh, hokein. Herald added subscribers: cfe-commits, kbarton, xazax.hun, nemanjai. The fix to the issue that `const char* p = ("foo")` is diagnosed as decay is to ignored the ParenCast. Resolves PR39583 Repository:

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

JonasToth added inline comments. Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:33 + const auto *VD = Result.Nodes.getNodeAs("var"); + const auto *Return = Result.Nodes.getNodeAs("return"); + diag(Return->getBeginLoc(), "address of static local variable %0 may not "

[PATCH] D54257: [clang-tidy] **Prototype** use AllTUsExecutors

JonasToth added a comment. Very interesting. As I am not very familiar with all the internals I do have a few questions :) Right now notes seem not be closely attached to their related warning. But within the there is a check, that a note is emitted after an error. Would it make sense to

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

JonasToth added inline comments. Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:33 + const auto *VD = Result.Nodes.getNodeAs("var"); + const auto *Return = Result.Nodes.getNodeAs("return"); + diag(Return->getBeginLoc(), "address of static local variable %0 may not "

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178 + return; +// Conversions to unsigned integer are well defined and follow modulo 2 +// arithmetic. gchatelet wrote: > JonasToth wrote: > > I

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. LG from my side. Please wait for feedback from @aaron.ballman or @alexfh before committing. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. > Could you please explain your motivation of catching clang-tidy stdout? > `--export-fixes` emits everything of `diagnostic` to YAML even the > `diagnostic` doesn't have fixes. I guess the reason is that you want code > snippets that you could show to users? If so,

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

JonasToth added a comment. I agree with @Eugene.Zelenko that this check should rather live in `bugprone-` as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module. Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:22 +void

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. > At least the clang-tidy quiet mode is trivial to implement. Maybe instead of > `--quiet` we could have `--stdout=` where `output_format` can > be one of `none`, `diag`, `yaml` and in the future possibly `json` (requested > here:

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. In https://reviews.llvm.org/D54141#1288930, @JonasToth wrote: > > Do you understand the proposal now? > > Yes better, I was under the impression that `clang-apply-replaments` is run > on the end and the YAMLs are kept until then. Now its clear. > I assume

[PATCH] D54168: [clang-tidy] Zircon fbl::move -> std::move

JonasToth added a comment. I think this check is ok in the current form, but does it really need to be in upstream clang-tidy? You said its for migration only, so it is not valuable for you for a long time either? Comment at:

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. In https://reviews.llvm.org/D54141#1288993, @Eugene.Zelenko wrote: > Reducing log file size is good idea, but I think will be also good idea to > count duplicates. This will allow to concentrate clean-up efforts on place > where most of warnings originate. Places

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. > Do you understand the proposal now? Yes better, I was under the impression that `clang-apply-replaments` is run on the end and the YAMLs are kept until then. Now its clear. I assume `--issue-diags` produce the same result as the normal diagnostic engine. That could

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added a comment. I think the check is really close. If the other reviewers want to take a look at it again, now is a good moment :) Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178 + return; +// Conversions to unsigned integer are

[PATCH] D54157: [clangd] [NFC] Fix clang-tidy warnings.

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. Fixing such warnings is ok without prior review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54157 ___ cfe-commits

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. > Maybe my suggestion was not clear. The yaml file generated by clang-tidy > contains not only replacements, but all diagnostics, even without a fixit. > > So, running `clang-apply-replacements --issue-diags the_new_file.yaml` would > issue the warnings/fixit hints

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. Thank you for the comment! In https://reviews.llvm.org/D54141#1288809, @steveire wrote: > This feature seems like a good idea. I started writing it too some months > ago, but then I changed tactic and worked on distributing the refactor over > the network instead.

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added a comment. > - The output of clang-tidy diagnostic is YAML, and YAML is not an > space-efficient format (just for human readability). If you want to save > space further, we might consider using some compressed formats, e.g. > llvm::bitcode. Given the reduced YAML result

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth added inline comments. Comment at: clang-tidy/tool/run_clang_tidy.py:1 +run-clang-tidy.py This simlink is required for my unittests, I don't know how to add the added tests in the `lit` test-suite so there is no change there yet. A bit of guidance

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth updated this revision to Diff 172726. JonasToth added a comment. - spurious change in my git Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54141 Files: clang-tidy/tool/run-clang-tidy.py clang-tidy/tool/run_clang_tidy.py

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

JonasToth created this revision. JonasToth added reviewers: alexfh, aaron.ballman, hokein, sammccall, lebedev.ri. Herald added subscribers: cfe-commits, xazax.hun, mgorny. `run-clang-tidy.py` is the parallel executor for `clang-tidy`. Due to the common header-inclusion problem in C++/C

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

JonasToth added a comment. In https://reviews.llvm.org/D54061#1288395, @sammccall wrote: > In https://reviews.llvm.org/D54061#1286956, @JonasToth wrote: > > > > Theoretically, we could replace `ClangTidyCheck::check` with > > > `ClangTidyCheck::run`, but I'm not sure it is worth, > > >

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20 + +static const char LoopName[] = "forLoopName"; +static const char loopVarName[] = "loopVar"; JonasToth wrote: > ztamas wrote: > > JonasToth wrote: > > > Please

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added inline comments. Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10 + + .. code-block:: c++ + Eugene.Zelenko wrote: > JonasToth wrote: > > ztamas wrote: > > > JonasToth wrote: > > > > the `.. code-block:: c++` is usually

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. last nits from my side (for now :)). If the other reviews could take a look at it as well, would be great. I am uncertain about the english in some comments @aaron.ballman finds all these language bugs ;) Comment at:

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + }

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:164 + while (i) { +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'int' to 'bool' [cppcoreguidelines-narrowing-conversions] + }

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

JonasToth added a comment. > Theoretically, we could replace `ClangTidyCheck::check` with > `ClangTidyCheck::run`, but I'm not sure it is worth, `ClangTidyCheck::check` > is a public API, and is widely-used (for all clang-tidy checks), replacing it > requires large changes (although it is

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84 "cppcoreguidelines-c-copy-assignment-signature"); +CheckFactories.registerCheck( +"cppcoreguidelines-avoid-c-arrays"); lebedev.ri

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth added inline comments. Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:44 + unless(anyOf(hasParent(varDecl(isExternC())), + hasAncestor(functionDecl(isExternC()) + .bind("typeloc"), JonasToth

[PATCH] D53771: [clang-tidy] Avoid C arrays check

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84 "cppcoreguidelines-c-copy-assignment-signature"); +CheckFactories.registerCheck( +"cppcoreguidelines-avoid-c-arrays"); lebedev.ri

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. Regarding the warning discussion: It is ok to have this check here in clang-tidy first and let it mature. Once we can handle all kind of loops and do not produce false positives this logic can move into the frontend. But in my opinion the warning-version should handle

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added inline comments. Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20 + +static const char LoopName[] = "forLoopName"; +static const char loopVarName[] = "loopVar"; ztamas wrote: > JonasToth wrote: > > Please move these variable in

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

JonasToth added a comment. In https://reviews.llvm.org/D53974#1285585, @ztamas wrote: > Just to make it clear. I think this check is in a good shape now. I mean it > catches usefull issues and also does not produce too many false positives, as > I see. Yes, I did not want to stop the patch

[PATCH] D54036: [fix][clang-tidy] fix for r345961 that introduced a test failure on Windows builds

JonasToth closed this revision. JonasToth added a comment. Thank you very much for the patch! Committed in r345995. Repository: rL LLVM https://reviews.llvm.org/D54036 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

<    1   2   3   4   5   6   7   8   9   10   >