[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: clang-tidy/tool/run-clang-ti

[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] 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 w

[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 c

[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 m

[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: clang-tidy/cppcoreguidelines/CMake

[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 variables.

[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 SI

[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] 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 (for

[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] 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, duratio

[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 `AST_MATCHER(Fun

[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 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 `D

[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] 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/DurationFactoryFloatCheck.cpp:61 + + if (SimpleArg) { diag(MatchedCall->getBeginLoc(), hwright wrote: > JonasToth wrote: > > hwright wrote: > > > JonasToth wrote: > > > > The diagnostic is not p

[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 > > code

[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: > JonasToth

[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 `.. c

[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: clang-tidy/abseil/DurationFactoryFloa

[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 definit

[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 t

[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] D55044: [clang-tidy] check for Abseil make_unique

JonasToth added a comment. In D55044#1312438 , @lebedev.ri wrote: > Thanks for working on this. > Semi-duplicate of D50852 Please see > discussion there. > It should not be an abseil-specific check, the prefix (`std`,`a

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

JonasToth added inline comments. Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:605 + return strncmp(SM.getCharacterData(T1.getLocation()), + SM.getCharacterData(T2.getLocation()), T1.getLength()) == 0; +} This operation could overflow

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

JonasToth added a comment. In D54757#1316899 , @donat.nagy wrote: > I applied this check to the llvm + clang codebase, and I was surprised to see > that it produced about 600 results (for comparison, the clang-tidy checks > which are enabled by default p

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

JonasToth added a comment. from my side no objections, mailing list did not react AFAIK (just in case your waiting for me until you recommit). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006 ___ cfe

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

JonasToth added a comment. Hi russell thank you for the patch! As I am not a clang-format reviewers these are only general things, and Nits anyway ;) Hope the reviewers I added will evaluate better. Comment at: lib/Format/WhitespaceManager.cpp:54 Tok.Decision = (Newlines >

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348161: [clang-tidy] Add the abseil-duration-comparison check (authored by JonasToth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D54737?vs

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

JonasToth added a comment. I had to revert and recommitted in rCTE348169 . `std::unordered_map` does not work, as `std::hash` is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best

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

JonasToth added a comment. > `int8` ? Did you mean `int8_t` or am I missing somthing ? Your right, but the solution I wrote did actually not work anyway.. I just specialized `std::hash<>` now. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54737/new/ https://revi

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

JonasToth added a comment. > A quick, tangentially related idea: Would it be useful to implement multiline > nolint sections (e.g. `//BEGINNOLINT` – `//ENDNOLINT` or something similar)? > This would be helpful for using clang-tidy on projects that contain some > automatically generated fragment

[PATCH] D55255: Fix a false positive in misplaced-widening-cast

This revision was automatically updated to reflect the committed changes. Closed by commit rL348341: Fix a false positive in misplaced-widening-cast (authored by JonasToth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55255?vs=17660

[PATCH] D55255: Fix a false positive in misplaced-widening-cast

JonasToth added a comment. Committed, Thank you for the patch! Was there a bug-report for this issue? If yes can you please close it/reference? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55255/new/ https://reviews.llvm.org/D55255 ___

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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE348343: [clang-tidy] new check: bugprone-branch-clone (authored by JonasToth, committed by ). Changed prior to commit: https://reviews.llvm.org/D54757?vs=176408&id=176772#toc Repository: rCTE Clang

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

JonasToth added a comment. I had to revert this patch because it broke (at least one) buildbot with an assertion-failure (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40436/steps/test/logs/stdio) Could you please take a look at it? I could not reproduce lo

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

JonasToth marked 2 inline comments as done. JonasToth added inline comments. Comment at: clang-tidy/utils/FixItHintUtils.cpp:35 +static bool isValueType(QualType QT) { return isValueType(QT.getTypePtr()); } +static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); } +

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

JonasToth added a comment. In D55245#1320546 , @hwright wrote: > Ping. > > I assume I've got the right reviewers here, but I've also been sending a > bunch of stuff your way lately, so if I'm overwhelming review capacity, > please let me know. Hi hyrum

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

JonasToth added a comment. Hi Sam, this patch "broke" `ExprMutAnalyzer`, at least it creates an assertion failure that seems harmless. Your thought on it would be great! The assertion in: `ASTMatchFinder.cpp +680` is triggered in the Analysis to figure out if something is mutated, because `Par

[PATCH] D55411: [clang-tidy] check for flagging using declarations not in the inner most namespace

JonasToth added a comment. Please upload the patch with full context. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55411/new/ https://reviews.llvm.org/D55411 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

JonasToth added a comment. Please upload with full context. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55410/new/ https://reviews.llvm.org/D55410 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one

JonasToth added a comment. Please upload with full context. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55409/new/ https://reviews.llvm.org/D55409 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

JonasToth added inline comments. Comment at: clang-tidy/add_new_check.py:213 match = re.search('Improvements to clang-tidy', line) +match_next = re.search('Improvements to include-fixer', line) +match_checker = re.search('- New :doc:`(.*)', line)

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationRewriter.cpp:21 +struct DurationScale2IndexFunctor { + using argument_type = DurationScale; + unsigned operator()(DurationScale Scale) const { Are you using `argument_type`? Browser searchin

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

JonasToth added a comment. I think this patch is fine, AFAIK these utility scripts are not tested directly but are just adjusted if they dont work as expected :) Did you test it with a fake new-check? If it does what we expect its fine :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

JonasToth added a comment. Hi MyDeveloperDay, thanks for the patch! Mostly stylistic comments. Would it make sense to attach the attribute to the implementation of the functions too? This check is definitly useful, but noisy. Do you see a change of another heuristic that could be applied to re

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. Do you have commit access? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55508/new/ https://reviews.llvm.org/D55508 ___ cfe-

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

JonasToth added a comment. In D55508#1325834 , @MyDeveloperDay wrote: > In D55508#1325758 , @JonasToth wrote: > > > LGTM. Do you have commit access? > > > I do not I'm afraid I will commit for you. CHANGES SINC

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

This revision was automatically updated to reflect the committed changes. Closed by commit rL348793: [clang-tidy] insert release notes for new checkers alphabetically (authored by JonasToth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

JonasToth added a comment. In D55508#1325960 , @Eugene.Zelenko wrote: > By the word, will be good idea to have script which check alphabetical order > and use it during build. Sometimes alphabetical order may be violated after > merge with trunk. I th

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

JonasToth added inline comments. Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29 +AST_MATCHER(CXXMethodDecl, isOverloadedOperator) { + // Don't put [[nodiscard]] front of operators. + return Node.isOverloadedOperator(); s/front/in front/ ===

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

JonasToth added inline comments. Comment at: clang-tidy/abseil/DurationRewriter.cpp:77 + getInverseForScale(Scale); + if (const auto *MaybeCallArg = selectFirst( + "e", hwright wrote: > JonasToth wrote: > > In Principle the `Node` could have multip

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

JonasToth added a comment. See PR39949 as well, as it seems to trigger the same or a similar problem. @ioeric and @klimek maybe could give an opinion, too? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54309/new/ https://reviews.llvm.org/D54309 __

[PATCH] D55541: Use the standard Duration factory matcher

JonasToth added a comment. In D55541#1326868 , @lebedev.ri wrote: > In D55541#1326867 , @JonasToth wrote: > > > Please remember to upload your patches with full context (i can highly > > recommend using `arc`, see

[PATCH] D55561: Stop stripping comments from AST matcher example code

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55561/new/ https://reviews.llvm.org/D55561 ___ cfe-commits mailing list cfe-com

[PATCH] D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer

JonasToth marked an inline comment as done. JonasToth added inline comments. Comment at: clang-tidy/utils/ExceptionAnalyzer.h:112 +/// throw, if it's unknown or if it won't throw. +enum State Behaviour : 2; + bjope wrote: > (post-commit comments) > > I'v

[PATCH] D58606: [clang-tidy] misc-string-integer-assignment: fix false positive

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. is there a bug or similar? If yes please mention it somewhere in the summary or so and close it :) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://review

[PATCH] D58609: [clang-tidy] bugprone-string-integer-assignment: Reduce false positives.

JonasToth added inline comments. Comment at: clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp:54 + (RHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects) || + LHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects))) { +return true; unn

[PATCH] D58137: [clang-tidy] Add the abseil-time-subtraction check

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. I am happy now :) Thank you for the patch, LGTM Comment at: test/clang-tidy/abseil-time-subtraction.cpp:12 + + d = absl::Hours(absl::ToUnixHours(t) - x); + // CHECK-M

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

JonasToth added a comment. > It took me far too long to come up with an update. Honestly, I was quite > demotivated as it turned out preventing name collisions of unqualifed names > after the rewrite was more difficult than I thought. Especially because this > error occurs sparsely when I test

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

JonasToth added a comment. Herald added a subscriber: jdoerfert. another ping. @alexfh and @aaron.ballman you commented on prior versions. Would be nice if you could take a (final) look at this patch! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

JonasToth added a comment. Welcome to the LLVM community and thank you for the patch lewmpk! Please add unit tests for the changes you made to the check. They live in `clang-tools-extra/test/clang-tidy/modernize-`. It is common to add a additional test-file to ensure the configuration option

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

JonasToth added a comment. I think this change is worth mentioning in the release notes as well (cte/docs/ReleaseNotes.rst) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58731/new/ https://reviews.llvm.org/D58731

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

JonasToth added a comment. In D58731#1413498 , @MyDeveloperDay wrote: > In D58731#1413476 , @lewmpk wrote: > > > I'm happy to land this ASAP but I don't have commit rights > > > So one of us could land it for you..

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return-type check

JonasToth added a comment. From my side only the nits are left. Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:94 + bool VisitDeclRefExpr(DeclRefExpr *S) { +const DeclarationName Name = S->getNameInfo().getName(); +if (!S->getQualifierLoc() && Name.isI

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

JonasToth updated this revision to Diff 188716. JonasToth marked 3 inline comments as done. JonasToth added a comment. - address review comments - rebase to master Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54395/new/ https://reviews.llvm.org/D54

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

JonasToth marked an inline comment as done. JonasToth added inline comments. Comment at: unittests/clang-tidy/AddConstTest.cpp:15 + +template alexfh wrote: > What's the point of default values of template arguments here? Wups, relict of older version. Reposito

[PATCH] D58609: [clang-tidy] bugprone-string-integer-assignment: Reduce false positives.

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58609/new/ https://reviews.llvm.org/D58609 _

[PATCH] D58764: [clang-tidy] ignore predefined expressions in cppcoreguidelines-pro-bounds-array-to-pointer-decay check

JonasToth added a comment. You can abondon this. A short justification (with reference to the other revision) on the bug report would be great! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58764/new/ https://reviews.llvm.org/D58764

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

This revision was automatically updated to reflect the committed changes. Closed by commit rL355093: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions (authored by JonasToth, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to com

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

JonasToth added a comment. Thank you for the patch! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58731/new/ https://reviews.llvm.org/D58731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

JonasToth added a comment. In D58731#1413888 , @MyDeveloperDay wrote: > @JonasToth I left a comment in the commit needed to fix the index.rst, which > I don't think your later review fixes, sphinx complained about the rst file > being an unreferenced oc

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

JonasToth accepted this revision. JonasToth added a comment. LGTM Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:196 + ReplacementText = " " + OverrideSpelling + " "; +} } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {

[PATCH] D58819: [clang-format] clang-format off/on not respected when using C Style comments

JonasToth added inline comments. Comment at: clang/lib/Format/Format.cpp:1792 +else if (Trimmed == "// clang-format on" || + Trimmed == "/* clang-format on */") FormattingOff = false; Should we allow ``` /* clang-format off It is just

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:35 +void UseRaiiLocksCheck::registerMatchers(MatchFinder *Finder) { + // lock_guards require c++11 or later + if (!getLangOpts().CPlusPlus11) If we allow boost, pre

[PATCH] D58819: [clang-format] clang-format off/on not respected when using C Style comments

JonasToth added inline comments. Comment at: clang/lib/Format/Format.cpp:1792 +else if (Trimmed == "// clang-format on" || + Trimmed == "/* clang-format on */") FormattingOff = false; MyDeveloperDay wrote: > JonasToth wrote: > > Should we a

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp:4 +// Mock implementation of std::mutex +namespace std { +struct mutex { lewmpk wrote: > JonasToth wrote: > > Please add more tests > > > > What happens for this? >

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp:90 + for (auto i = 0; i < 3; i++) { +m.lock(); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use RAII I got another one that I think defeats the analysis: `

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp:90 + for (auto i = 0; i < 3; i++) { +m.lock(); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use RAII lewmpk wrote: > JonasToth wrote: > > I got another one

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

JonasToth added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp:90 + for (auto i = 0; i < 3; i++) { +m.lock(); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use RAII JonasToth wrote: > lewmpk wrote: > > JonasToth wrote: >

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

JonasToth added a comment. IMHO the check is close to being finished. please address the notes and mark them as done if finished with it. So its clear to see whats outstanding. In my opinion the user-facing documentation misses a "Limitations" sections that shows the artificial `goto` example, t

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

JonasToth added a comment. @lebedev.ri @kallehuttunen what do you think of putting this into context of the proposal (i believe its being standardized with c++20) of `operator==() = default` and the spaceship-operator. This check could serve as a basis to modernize the `operator==` to the defaul

[PATCH] D59318: [clang-tidy] add an overload for diag method that allows users to provide a diagnostic name rather than using the check name when building a diagnostic.

JonasToth added a comment. What is the reason you want this change to happen? I think this gives the chance to create inconsistencies which we should avoid. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59318/new/ https://reviews.llvm.org/D59318

[PATCH] D56343: [clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py

JonasToth added a comment. From my side its LGTM, but I would let @serge-sans-paille accept, as he is probably more familiar with python then I am. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56343/new/ https://reviews.llvm.org/D56343 __

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

JonasToth added a comment. ping :) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54395/new/ https://reviews.llvm.org/D54395 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

JonasToth added a comment. In D57108#1433087 , @baloghadamsoftware wrote: > Ping! Any news regarding this patch? Its on the todolist, but i currently have many things to do in my real life, so no time for clang-tidy development :( Repository: rCTE

[PATCH] D59540: [clang-tidy] [PR41119] readability-identifier-naming incorrectly fixes lambda capture

JonasToth added a comment. What happens on `[=]() ...`, direct capture `[&Columns]()...` and `[Columns]()...`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59540/new/ https://reviews.llvm.org/D59540 ___ cfe-commits mailing list cfe-commit

[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

JonasToth added a comment. Why not having normal overloads? The analysis for `Stmt` is implemented with the private methods. Explicit template specialization is a bit overkill and so easily understood (but not too complex in this case either).l Repository: rCTE Clang Tools Extra CHANGES SIN

[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

JonasToth added a comment. > Looks like pointless code duplication that is easily avoidable. True, but I would prefer the refactoring to be private then (so the template stuff with the boilerplate) and a simple overloading interface dispatching to the templated stuff. In the end I think the pub

[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

JonasToth added inline comments. Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:226 +ExceptionAnalyzer::ExceptionInfo +ExceptionAnalyzer::analyzeBoilerplate(const T *Node) { + ExceptionInfo ExceptionList; lebedev.ri wrote: > Please bikeshed on the name. I do

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

JonasToth added a comment. I think in general this is a good direction. What do you think about other heuristics? E.g. one could say that a vector will not contain more then X GB or similar. That is probably more complicated to figure out though. Comment at: docs/clang-tidy/c

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

JonasToth added a comment. I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too? I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone a

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

JonasToth added a comment. > I think it's the easiest way to specify the bits of the ineteger type to > limit the catches. In real life, I met with this overflow / infinite loop > problem with 16-bit short type, so I think the real use cases are 8 and 16 > bit integers. It seems intuitive to me

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

JonasToth added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:124 + ` now supports + `MagnitudeBitsUpperLimit` option. + Please add the new default here as its a change in behaviour of the check. Comment at: clang-tool

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:33 + + Upper limit for the magnitue bits of the loop variable. If it's

[PATCH] D60453: ClangTidy: Avoid mixing stdout with stderror when dealing with a large number of files.

JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. How does that happen to be a problem? It feels that `stdout.write` and `stderr.write` should be unconnected? (if you know the reason I would like to know it too :)) Please note, that `ru

[PATCH] D60139: [clang-tidy] Add bugprone-placement-new-target-type-mismatch check

JonasToth added a comment. Hey Dennis, my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:102 "bugprone-too-small-loop-variable"); +CheckFactories.registerCheck( +"bugprone-unhandled-self-assignment"); please order this list a

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