[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri updated this revision to Diff 113228. lebedev.ri added a comment. Rebased. Repository: rL LLVM https://reviews.llvm.org/D36836 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp clang-tidy/readability/FunctionCognitiveCom

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri updated this revision to Diff 113230. lebedev.ri added a comment. Rebased the correct branch this time... When splitting off https://reviews.llvm.org/D36892, and addressing the other review note in this Revision, i slightly messed up the local branches, so the wrong code got rebased p

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

lebedev.ri added a comment. In https://reviews.llvm.org/D36892#856469, @aaron.ballman wrote: > Instead of CHECK-NOTES, do we want to extend CHECK-MESSAGES to handle `note` > in addition to `warning` and `error`? If i change `CHECK-MESSAGES` to also require all the notes to be checked, a *lot*

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

lebedev.ri added a comment. In https://reviews.llvm.org/D36892#856501, @JonasToth wrote: > Note can be handled right now as well. Yes. Adding this new prefix is about adding `-implicit-check-not=notes`. I.e. if you use `CHECK-MESSAGES`, then it will only enforce you to have check-lines for all

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

lebedev.ri updated this revision to Diff 113254. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Reword the 'no magic found' error message. Repository: rL LLVM https://reviews.llvm.org/D36892 Files: test/clang-tidy/check_clang_tidy.py Index: test/clang-tidy/check

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. Any status update here? :) I generally do see a benefit in this check, because `-Wcast-align` (at least currently?) does not warn on `reinterpret_cast<>()`. Repository: rL LLVM https://reviews.llvm.org/D33826 ___ cfe-

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#856619, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D33826#856610, @lebedev.ri wrote: > > > Any status update here? :) > > I generally do see a benefit in this check, because `-Wcast-align` (at > > least currently?) does no

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a subscriber: rjmccall. lebedev.ri added a comment. In https://reviews.llvm.org/D33826#856619, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D33826#856610, @lebedev.ri wrote: > > > Any status update here? :) > > I generally do see a benefit in this check, because `-Wcast-

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#772676, @aaron.ballman wrote: > How does this check compare with the -Wcast-align diagnostic in the Clang > frontend? I believe that warning already covers these cases, so I'm wondering > what extra value is added with this check?

[PATCH] D37381: Fix regression in special member definitions under SuppressAllDiagnostics

lebedev.ri added a comment. 1. Please upload path with full context (-U) 2. Tests? https://reviews.llvm.org/D37381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37381: Fix regression in special member definitions under SuppressAllDiagnostics

lebedev.ri added a comment. In https://reviews.llvm.org/D37381#858800, @brad.king wrote: > > Tests? > > `make check` didn't regress from this. Which is the problem. Say this gets committed, and then someone else for some reason reverts part of the patch. Normally, tests should catch that. But

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added a comment. I understand that everyone is likely busy with 5.0, but still, give me at least some feedback, please :) ping Repository: rL LLVM https://reviews.llvm.org/D36836 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri created this revision. lebedev.ri added a project: clang. This is a first half(?) of a fix for the following bug: https://bugs.llvm.org/show_bug.cgi?id=34147 (gcc -Wtype-limits) GCC's -Wtype-limits does warn on comparison of unsigned value with signed zero (as in, with 0), but clang on

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri updated this revision to Diff 114172. lebedev.ri added a comment. Added ReleaseNotes.rst note Repository: rL LLVM https://reviews.llvm.org/D37565 Files: docs/ReleaseNotes.rst lib/Sema/SemaChecking.cpp test/Sema/outof-range-constant-compare.c Index: test/Sema/outof-range-con

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri updated this revision to Diff 114221. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Address review notes. Repository: rL LLVM https://reviews.llvm.org/D37565 Files: docs/ReleaseNotes.rst lib/Sema/SemaChecking.cpp test/Sema/outof-range-constant-comp

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8592 + bool BadR = !RType->isIntegerType() || RType->isSignedIntegerType() || + RHS->isKnownToHaveBooleanValue(); + rjmccall wrote: > Please extract a function which computes th

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8879 + if (IsComparisonConstant) return AnalyzeImpConvsInComparison(S, E); rjmccall wrote: > lebedev.ri wrote: > > rjmccall wrote: > > > Part of the purpose of checking for signed c

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri planned changes to this revision. lebedev.ri added inline comments. Comment at: test/Sema/outof-range-constant-compare.c:41 +if (a < 0xUL) +return 0; +if (a <= 0xUL) rjmccall wrote: > Hmm. I think this s

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri updated this revision to Diff 114249. lebedev.ri marked 4 inline comments as done. lebedev.ri added a comment. Address review notes. Repository: rL LLVM https://reviews.llvm.org/D37565 Files: docs/ReleaseNotes.rst lib/Sema/SemaChecking.cpp test/Sema/compare.c test/Sema/out

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

lebedev.ri updated this revision to Diff 114260. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Address review notes. Repository: rL LLVM https://reviews.llvm.org/D37565 Files: docs/ReleaseNotes.rst lib/Sema/SemaChecking.cpp test/Sema/compare.c test/Sema/out

[PATCH] D37620: [Sema] Put tautological comparison of unsigned and zero into it's own flag

lebedev.ri created this revision. lebedev.ri added a project: clang. As requested by Sam McCall. $ /build/llvm-build-Clang-release/./bin/clang -c /build/clang/test/Sema/outof-range-constant-compare.c /build/clang/test/Sema/outof-range-constant-compare.c:40:11: warning: comparison of unsigned

[PATCH] D37620: [Sema] Put tautological comparison of unsigned and zero into it's own flag

lebedev.ri updated this revision to Diff 114351. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Added test. Repository: rL LLVM https://reviews.llvm.org/D37620 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td test/Se

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri created this revision. lebedev.ri added a project: clang. As requested by Sam McCall: > Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX) > The warning strongly suggests that the enum < 0 check has no effect > (for enums with nonnegative ranges). > Clang doesn't see

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri updated this revision to Diff 114387. lebedev.ri added a subscriber: jroelofs. lebedev.ri added a comment. Rework as per @jroelofs's suggestion to have just one `switch`/`if` cascade that operates on `BinaryOperatorKind` Repository: rL LLVM https://reviews.llvm.org/D37629 Files:

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri updated this revision to Diff 114396. lebedev.ri added a comment. And finish reducing the code by for-range-loop`ing over array + use `std::array`. Repository: rL LLVM https://reviews.llvm.org/D37629 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/Diagnosti

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri added a comment. In https://reviews.llvm.org/D37629#865068, @lebedev.ri wrote: > And finish reducing the code by for-range-loop`ing over array + use > `std::array`. I will need to fix handling of the second edge-case (comparison with max unsigned value or with min/max for signed va

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#866133, @NorenaLeonetti wrote: > In https://reviews.llvm.org/D33826#861226, @aaron.ballman wrote: > > > Have you run this check over any large code bases to see what the quality > > of the diagnostics are? > > > `-checks=-*,cert-exp3

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#866161, @JonasToth wrote: > There is an exception to the general rule (EXP36-C-EX2), stating that the > result of `malloc` and friends is allowed to be casted to stricter > alignments, since the pointer is known to be of correct ali

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri updated this revision to Diff 114549. lebedev.ri added a comment. @alexfh et al: friendly ping :) Repository: rL LLVM https://reviews.llvm.org/D36836 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp clang-tidy/readabili

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#868085, @aaron.ballman wrote: > In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D33826#866161, @JonasToth wrote: > > > > > There is an exception to the general rule (EXP36-C-EX2), statin

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#868185, @aaron.ballman wrote: > In https://reviews.llvm.org/D33826#868167, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D33826#868085, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrot

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri updated this revision to Diff 114861. lebedev.ri added a comment. Aaaand revert back to dump approach with code duplication as requested :) (For the second part of the fix in another differential, if the reverted approach with struct turns out to be better, i guess i will use it) Re

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. In https://reviews.llvm.org/D33826#868185, @aaron.ballman wrote: > Ah, I think I'm catching on to the point you're raising (thank you for > the patience). If the return type is void *, we don't have enough > information to sensibly diagnose (not without data flow ana

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

lebedev.ri added a comment. What about GNU extension `case 1 ... 3:` ? https://reviews.llvm.org/D37808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

lebedev.ri added a comment. In https://reviews.llvm.org/D37808#869879, @JonasToth wrote: > In https://reviews.llvm.org/D37808#869823, @lebedev.ri wrote: > > > What about GNU extension `case 1 ... 3:` ? > > > Strictly speaking, the coding standard (which should be enforced by the > patch) require

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri updated this revision to Diff 115482. lebedev.ri marked 13 inline comments as done. lebedev.ri added a comment. Address most of the @JonasToth's review notes. Repository: rL LLVM https://reviews.llvm.org/D36836 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readabilit

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:62 +std::pair Process() const { + assert(C != Criteria::None && "invalid criteria"); + JonasToth wrote: > You acces `Critera` always scoped. I think

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#873032, @JonasToth wrote: > I built the patch locally, here is my output: > > The Unittest failed for me: `Neither CHECK-FIXES nor CHECK-MESSAGES found in > the input` > > Script call: > `/usr/bin/python2.7 > /home/jonas/opt/llvm/

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

lebedev.ri added a comment. In https://reviews.llvm.org/D37808#869810, @Eugene.Zelenko wrote: > In https://reviews.llvm.org/D37808#869612, @JonasToth wrote: > > > In https://reviews.llvm.org/D37808#869602, @Eugene.Zelenko wrote: > > > > > I think will be good idea to extend -Wswitch diagnostics.

[Diffusion] rL302247: Introduce Wzero-as-null-pointer-constant.

lebedev.ri added subscribers: thakis, cfe-commits, dblaikie, malcolm.parsons, hans, lebedev.ri. lebedev.ri raised a concern with this commit. lebedev.ri added a comment. Hi @thakis! Are you planning on addressing the kind-of false-positives this diagnostic produces? It should not warn if `0` is:

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri updated this revision to Diff 115547. lebedev.ri marked 13 inline comments as done. lebedev.ri added a comment. Address even more @JonasToth's review notes. - Fixed docs - Validated the implementation (the testcase) against the "reference" implementation - Fixed discrepancies in the i

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114 +const std::array CognitiveComplexity::Msgs = {{ +// B1 + B2 + B3 +"+%0, including nesting penalty of %1, nesting level increased to %2", + +// B1 + B2 +

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114 +const std::array CognitiveComplexity::Msgs = {{ +// B1 + B2 + B3 +"+%0, including nesting penalty of %1, nesting level increased to %2", + +// B1 + B2 +

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#873246, @JonasToth wrote: > For my part the current state is ok. @JonasToth thank you for the review! > but @alexfh and @aaron.ballman should do their review before committing. +1 :) Now what one full review is done, it may be eas

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri added a comment. Ping, would be really nice to get this going :) Would unblock me for working on the second half of the fix for https://bugs.llvm.org/show_bug.cgi?id=34147 Repository: rL LLVM https://reviews.llvm.org/D37629 ___ cfe-co

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri updated this revision to Diff 115877. lebedev.ri added a comment. 1. Correctly added `TautologicalUnsignedEnumZeroCompare` to `TautologicalCompare` group (sigh) 2. Hopefully fixed spelling of the comments and added a bit better comments 3. Fixed variable names to comply with the coding

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri updated this revision to Diff 115890. lebedev.ri added a comment. Simplify `CheckTautologicalComparisonWithZero()` as per review notes. Less LOC. Repository: rL LLVM https://reviews.llvm.org/D37629 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSe

[PATCH] D37629: [Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare

lebedev.ri reopened this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Reverted due to buildbot failures. Need to investigate. Repository: rL LLVM https://reviews.llvm.org/D37629 ___ cfe-commits mailing li

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

lebedev.ri created this revision. lebedev.ri added a project: clang. Currently, clang only diagnoses completely out-of-range comparisons (e.g. `char` and constant `300`), and comparisons of unsigned and `0`. But gcc also does diagnose the comparisons with the `std::numeric_limits<>::max()` / `st

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#877636, @alexfh wrote: > > The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource > > specification version 1.2 (19 April 2017), > > Err, "I did ask them, and received an answer that it is it can be implemented > in cl

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

lebedev.ri added a comment. Tried stage2 build, so far only one warning. But found out that `ubsan_value.h`'s `typedef s128 SIntMax;` crashes this code because class LLVM_NODISCARD APSInt : public APInt { ... /// \brief Get the correctly-extended \c int64_t value. int64_t getExtValue

[PATCH] D38171: Implement clang-tidy check aliases.

lebedev.ri added a comment. On a somewhat related note, since this is already talking about aliases I feel like the current handling of the clang-tidy check aliases needs adjusting. If one enables all the checks (`Checks: '*'`), and then disables some of them on check-by-check basis, if the dis

[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

lebedev.ri updated this revision to Diff 116465. lebedev.ri added a comment. - Fixed handling of types > 64 bit (e.g. `__int128`), testcase added - Stage-2 build now fully passes (only one new warning, https://reviews.llvm.org/D38132) - Updated ReleaseNotes.rst Repository: rL LLVM https://re

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

lebedev.ri added a comment. Nice! /home/eenitot/new_llvm/llvm/lib/Support/ConvertUTFWrapper.cpp:38:26: warning: do not cast pointers into more strictly aligned pointer types [cert-exp36-c] UTF16 *targetStart = reinterpret_cast(ResultPtr); ^ Hmm, the text is we

[PATCH] D62729: [ARM] Fix recent breakage of -mfpu=none.

lebedev.ri added inline comments. Comment at: clang/test/CodeGen/arm-mfpu-none.c:2 +// REQUIRES: arm-registered-target +// RUN: %clang -target arm-none-eabi -mcpu=cortex-m4 -mfpu=none -S -o - %s | FileCheck %s + Generally clang codegen tests should test `-emit-l

[PATCH] D62730: [RFC] Alternate implementation of D53157 IRBuilder for Constrained FP using enumeration vs MDNode and add support for fp-model and fp-speculation language options

lebedev.ri added a comment. New files are missing header blurbs; the **precise** explanation of what is what is missing (should be in `doc/`, not patch description) Comment at: include/llvm/IR/FPState.h:9-31 + enum FPModelKind { +FPM_Off, +FPM_Precise, +FPM_Strict

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

lebedev.ri added a comment. Documentation missing. All the blurb from patch description should be in `doc/` Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62731/new/ https://reviews.llvm.org/D62731 ___ cfe-commits mail

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

lebedev.ri added a comment. I don't know which patch will proceed; but they weren't added already, this seems to be missing documentation changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

lebedev.ri added a comment. Some basic thoughts Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:63 + "cast from %0 to %1 may lead to access memory based on invalid memory " + "layout; pointed to type is wider than the allocated

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

lebedev.ri added a comment. Also, this kinda feels like this shouldn't be a single check. 1. I'm very much looking forward the alignment part of this, the `reinterpret_cast<>()` specifically. It would be good to have a switch to also diagnose casts from `void*` here. But indeed, if it comes fro

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

lebedev.ri added a comment. In D48866#1527506 , @steakhal wrote: > The problem with the `-Wcast-align` is that it will only fire for C-style > bitcast expressions, not for `reinterpret_cast` ones. example > > Does anyone k

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40 +return; + Finder->addMatcher(varDecl().bind("var"), this); +} Most of the matching should be done here, not in `check()`. Repository

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

lebedev.ri added a comment. .. or? Is this fixing a current test failure? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62850/new/ https://reviews.llvm.org/D62850 ___ cfe-commits mailing list cfe-commi

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

lebedev.ri added a comment. In D62850#1529081 , @russell.gallop wrote: > > .. or? > > I'm not sure what you mean. > > > Is this fixing a current test failure? > > No. The test is not failing, but it is not doing what was intended as these > builtins are

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

lebedev.ri added a comment. In D62850#1529588 , @russell.gallop wrote: > > We support non immediate on these because gcc does. > > Thanks. Your comment crossed in mid-air. Okay, so is this test worth > changing, or should it have both versions (immediate

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

lebedev.ri added inline comments. Comment at: clang-tidy/readability/RedundantExternCheck.cpp:30 + + if (FD->getStorageClass() != SC_Extern) +return; Can you do that in `registerMatchers()`? Comment at: clang-tidy/readability/RedundantExte

[PATCH] D62970: [clang-doc] De-duplicate comments

lebedev.ri added a comment. Test? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62970/new/ https://reviews.llvm.org/D62970 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

lebedev.ri added a comment. By the way, much to my surprise, this didn't start diagnosing the loop i expected it to start diagnosing: https://godbolt.org/z/lsJTSS This is expected? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61827/new/ https://re

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

lebedev.ri added a comment. Hmm, but there already is clang's `-Wglobal-constructors`, that fires on some of these: https://godbolt.org/z/rSnNuu You are sure those extra warning this check produces ontop of `-Wglobal-constructors` are correct? If so, maybe `-Wglobal-constructors` should be exten

[PATCH] D63048: Update __VERSION__ to remove the hardcoded 4.2.1 version

lebedev.ri added inline comments. Comment at: lib/Frontend/InitPreprocessor.cpp:610 + Builder.defineMacro("__VERSION__", "\"" CLANG_VERSION_STRING + " Compatible " + Twine(getClangFullCPPVersion()) + + "\""); So how does

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

lebedev.ri added a comment. You need to upload the entire patch, not as compared to the previous patch. Without seeing the tests - what version checks does this have? It shouldn't fire if the googletest version is the one before that rename. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

lebedev.ri added inline comments. Comment at: clang-tidy/readability/RedundantExternCheck.cpp:43-44 + + if (FD->getBeginLoc().isMacroID()) +return; + koldaniel wrote: > lebedev.ri wrote: > > Similarly, do this in `registerMatchers()` > Could you please help

[PATCH] D63329: Allow static linking of libc++ on Linux, just like -static-libstdc++

lebedev.ri added a comment. Tests? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63329/new/ https://reviews.llvm.org/D63329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailm

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

lebedev.ri added a comment. Missing tests. Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:120 +} // namespace clang \ No newline at end of file please add all the missing newlines Comment at: clang-tools-extra

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

lebedev.ri added a comment. In D63324#1543607 , @Blackhart wrote: > Modernize memcpy only if C++20 is enabled ... why? This is also missing documentation,releasenotes changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63324/new/ https://re

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

lebedev.ri added a comment. In D63324#1543626 , @Blackhart wrote: > In D63324#1543609 , @lebedev.ri > wrote: > > > In D63324#1543607 , @Blackhart > > wrote: > > > > > Moder

[PATCH] D63088: [clang-tidy] misc-unused-parameters: don't comment out parameter name for C code

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Seems obviously correct, with the nit. Comment at: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp:141-156 + // Cannot remove parameter for non-local fun

[PATCH] D63089: [clang] Warn on implicit boolean casts in more contexts (PR34180)

lebedev.ri added a comment. Please always upload all patches with full context (`-U9`) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63089/new/ https://reviews.llvm.org/D63089 ___ cfe-commits maili

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:232 { +llvm::TimeTraceScope TimeScope("Frontend", StringRef("")); PrettyStackTraceString CrashInfo("Per-file LLVM IR generation"); This looks more like `Fron

[PATCH] D43500: [clang-tidy]: modernize-use-default-member-init: Remove trailing comma and colon.

lebedev.ri added a comment. In D43500#1546077 , @jdemeule wrote: > In D43500#1244518 , @JonasToth wrote: > > > What is the status here? Will you continue to work on this patch @jdemeule > > (which would be great!)

[PATCH] D63324: [clang-tidy] Replace memcpy by std::copy

lebedev.ri added a comment. Still missing tests Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceMemcpyByStdCopy.cpp:34-35 + callExpr(hasDeclaration(functionDecl(hasName("memcpy"), + isExpansionInSystemHeader())), +

[PATCH] D63497: Add support for openSUSE RISC-V triple

lebedev.ri added a reviewer: asb. lebedev.ri added a comment. Nice :) It is also a good idea to upload patches with full context (`-U9`) Comment at: llvm/unittests/ADT/TripleTest.cpp:333 + T = Triple("riscv64-suse-linux"); + EXPECT_EQ(Triple::riscv64, T.getArch());

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:250-252 + // Finish "Frontend" section starting inside clang::ParseAST() + if (llvm::timeTraceProfilerEnabled()) +llvm::timeTraceProfilerEnd(); I think i'm missing

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:250-252 + // Finish "Frontend" section starting inside clang::ParseAST() + if (llvm::timeTraceProfilerEnabled()) +llvm::timeTraceProfil

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

lebedev.ri added a comment. Hmm, i'm only now noticing how fraglie this looks. How do we know that `clang::ParseAST` will only ever be called with `BackendConsumer` `ASTConsumer`? How do we know that `BackendConsumer` will only ever be `ASTConsumer` from `clang::ParseAST`? Repository: rG LLV

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

lebedev.ri added a comment. I've always been frustrated at how clang just gives up as soon as it sees a macro. At best that should be controlled with some command-line flag. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63423/new/ https://reviews.llvm.org/D63423

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

lebedev.ri added a comment. In D63423#1550728 , @jfb wrote: > In D63423#1550725 , @lebedev.ri > wrote: > > > I've always been frustrated at how clang just gives up as soon as it sees a > > macro. > > At best that

[PATCH] D63616: Implement `-fsanitize-coverage-whitelist` and `-fsanitize-coverage-blacklist` for clang

lebedev.ri added a comment. Test coverage missing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63616/new/ https://reviews.llvm.org/D63616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D64592: [OpenMP] Fix unified memory implementation for multiple compilation units

lebedev.ri added a comment. Description lacking. Why is that bad? How does this play wrt reproducibility of the output? Normally value names in IR are completely discarded in Release build mode, why do they matter here? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D64607: [clang-tidy] Fix crash on end location inside macro

lebedev.ri added a comment. Test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64607/new/ https://reviews.llvm.org/D64607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

lebedev.ri added a comment. Herald added a project: LLVM. Is this doc supposed to be autogenerated somehow? I believe `MacroDefinitionCase` (D21020 ) is missing from it, maybe others. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5

[PATCH] D64644: Fixes a clang frontend assertion failure (bug 35682)

lebedev.ri added a comment. Test? Comment at: clang/lib/Sema/SemaTemplate.cpp:726 +return ExprError(); + else +return DependentScopeDeclRefExpr::Create(Context, std::move(QualifierLoc), no else after return Repository: rG LLVM Github Monorepo CHAN

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

lebedev.ri added a comment. Test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

lebedev.ri added inline comments. Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1 +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash Why do you need `-O1`? Comment at

[PATCH] D64671: New clang-tidy check: misc-init-local-variables

lebedev.ri added a comment. This certainly needs more tests: macros, `-x C`, ??? Comment at: clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp:26-29 + if(!MatchedDecl->isLocalVarDecl()) +return; + if(MatchedDecl->hasInit()) +return; Can ma

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. Sorry, i still don't like the test. You want to check the produced IR. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

lebedev.ri added a comment. Hm, i have a question about this fix. As it can be seen the C++17 code is successfully codegened by clang to LLVM IR, and the actual failure is in LLVM middle-end optimization pass: https://godbolt.org/z/P3RB23 1. Please file a bug about that pass crash, include that

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

lebedev.ri added a comment. In D64656#1584424 , @lebedev.ri wrote: > Hm, i have a question about this fix. > As it can be seen the C++17 code is successfully codegened by clang to LLVM > IR, and the actual failure is in LLVM middle-end optimization pass:

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

lebedev.ri resigned from this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. In D64656#1584437 , @oydale wrote: > My understanding of the issue is that clang emits incorrect IR. Without my > fix and when disabling t

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. Thanks for working on this! You want to use `clang/Analysis/Analyses/ExprMutationAnalyzer.h`. See also: D51870 Repository: rCTE Clang Tool

<    5   6   7   8   9   10   11   12   13   14   >