[PATCH] D43780: [Tooling] [1/1] Refactor FrontendActionFactory::create() to return std::unique_ptr<>

lebedev.ri abandoned this revision. lebedev.ri added a comment. Herald added subscribers: kadircet, arphaman, jkorous. Herald added a project: LLVM. Superseded by D66960 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D43780/new/ htt

[PATCH] D66947: Changed FrontendActionFactory::create to return a std::unique_ptr

lebedev.ri added a comment. Commented on wrong patch. In D66960#1651198 , @lebedev.ri wrote: > This is missing documentation changes. > And this likely would be good to mention in releasenotes. Repository: rL LLVM CHANGES SINCE LAST ACTION http

[PATCH] D66960: [Tooling] Migrated APIs that take ownership of objects to unique_ptr

lebedev.ri added a comment. In D66960#1651243 , @gribozavr wrote: > In D66960#1651198 , @lebedev.ri > wrote: > > > This is missing documentation changes. > > > Could you point out such places? I tried to remove "ta

[PATCH] D66862: Make lround builtin constexpr (and others)

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/math-builtins.cpp:5 +{ + constexpr float f = 12345.6789; + Needs more tests This looks like you always round up Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D66862: Make lround builtin constexpr (and others)

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/math-builtins.cpp:1 +// RUN: %clang_cc1 -std=c++11 %s + -verify ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/

[PATCH] D66862: Make lround builtin constexpr (and others)

lebedev.ri added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9616 +return EvaluateFloat(E->getArg(0), Val, Info) && + Success(lround(Val.convertToDouble()), E); + } zoecarver wrote: > rsmith wrote: > > This assumes that the host `do

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

lebedev.ri resigned from this revision. lebedev.ri added a comment. Looks about right now, thanks for unbreaking it. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5419-5420 // b relational-op var // if (!S) { + SemaRef.Diag(DefaultLoc, diag::err_omp_loop_no

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

lebedev.ri added a comment. Trying to start reviewing this. The llvm side of the patch is self contained; clang patch should be split into a dependent review. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626 + unsigned Line, Column; + bool BadDebugInfo = false; + FullSou

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

lebedev.ri added a comment. Missing tests for `__builtin_unpredictable()`. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626 + unsigned Line, Column; + bool BadDebugInfo = false; + FullSourceLoc Loc = paulkirth wrote: > lebedev.ri wrote: > > This should be

[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc

lebedev.ri added a comment. Tests missing. Is that what gcc does? I'd personally thought those should be internalized. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67058/new/ https://reviews.llvm.org/D67058 ___ cfe-

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218507. lebedev.ri added a comment. NFC, fixup docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 Files: clang/docs/ReleaseNotes.rst clang/docs/UndefinedBehav

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri created this revision. lebedev.ri added reviewers: vsk, filcab, rsmith, aaron.ballman, vitalybuka, rjmccall, Sanitizers. lebedev.ri added projects: clang, Sanitizers. Herald added subscribers: arphaman, dberris. Herald added a project: LLVM. Quote from http://eel.is/c++draft/expr.add#4

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1656189 , @aaron.ballman wrote: > One fear I have with this is in expansions of the `offsetof` macro, where it > is a common implementation strategy to cast a null pointer to be of the > correct type when calculati

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218519. lebedev.ri added a comment. Add test that show that `__builtin_offsetof()` / `((uintptr_t)(&(((S *)nullptr)->y)))` are already not sanitized. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218533. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Reworded `(ptr - intptr_t(ptr)) -> nullptr` ubsan message to be less specific. Currently, `EmitCheckedInBoundsGEP()` is used sparsely, a lot of `GEP inbounds` are created dire

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked 2 inline comments as done. lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likew

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218542. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. `clang/docs/ReleaseNotes.rst`: also mention C17 verse in addition to C++ verse. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:59 + ``getelementptr inbounds`` can not change the null status of a pointer, + meaning it can not produce non-null pointer given null base pointer, and + likewise given non-null base pointer it can not pr

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177 ``-fsanitize=undefined``. + - ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset`` + and ``pointer-overflow``. vsk wrote: > Why does this

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218556. lebedev.ri marked 7 inline comments as done. lebedev.ri added a comment. @vsk thank you for taking a look! Review notes addressed, except grouping one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67

[PATCH] D67135: [clang-tidy] performance-inefficient-vector-operation: Support proto repeated field

lebedev.ri added a comment. I'm not sure this is the correct approach. I'd think the check instead should be parametrized, so this patch should become just extending the config. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67135/new/ https://revie

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

lebedev.ri added a comment. In D64146#1657117 , @svenvh wrote: > Shared library builds seem to be broken indeed. I tried fixing by adding > `Support` and `clangAST` as dependencies for `clangInterp`, but that creates > a cyclic dependency between `clang

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

lebedev.ri added a comment. In D64146#1657146 , @lebedev.ri wrote: > In D64146#1657117 , @svenvh wrote: > > > Shared library builds seem to be broken indeed. I tried fixing by adding > > `Support` and `clangAST` a

[PATCH] D67159: [clang] New __attribute__((__clang_builtin)).

lebedev.ri added a comment. It somehow doesn't seem entirely good to provide a way to mark arbitrary function in sources as a builtin.. Why can't those MVE builtins be implemented similarly like all the existing builtins? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

lebedev.ri added a comment. In D64146#1657473 , @nand wrote: > Merged clangInterp and clangAST into a single library while keeping Interp in > a separate folder. > This is required since clangInterp needs access to the AST, but the intry > points to the

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218720. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Merged the check into `pointer-overflow`, revisited test coverage. Using `EmitCheckedInBoundsGEP()` in more places TBD. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720 +// 2) The sign of the difference between the computed address and the base +// pointer matches the sign of the total offset. +llvm:

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 218792. lebedev.ri marked 25 inline comments as done. lebedev.ri added a comment. @vsk thank you for taking a look! I believe i have addressed or replied to all the comments. Other than the optimization comment, i currently don't believe that is a sound tr

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4538 + llvm::Value * /*OffsetOverflows*/> +EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal, + llvm::LLVMContext &VMContext, CodeGenModule &CGM,

[PATCH] D67159: [clang] New __attribute__((__clang_builtin)).

lebedev.ri added a comment. In D67159#1659084 , @simon_tatham wrote: > On the general discomfort with this attribute existing: I'd be happy to lock > it down, or mark it as "not recommended" in some way, if that's any help. I > don't personally intend a

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1659882 , @rupprecht wrote: > > Still think this looks good. Have you tried running this on the llvm test > > suite, or some other interesting corpus? Would be curious to see any > > pre/post patch numbers. > > I fin

[PATCH] D67265: [clang-tidy] Magic number checker shouldn't warn on user defined string literals

lebedev.ri added inline comments. Comment at: clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp:1 -// RUN: %check_clang_tidy %s readability-magic-numbers %t \ +// RUN: %check_clang_tidy -std=c++14 %s readability-magic-numbers %t \ // RUN: -config='{CheckOptions: \

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri marked 5 inline comments as done. lebedev.ri added inline comments. Herald added a subscriber: ychen. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720 +// 2) The sign of the difference between the computed address and the base +// pointer matches the si

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri updated this revision to Diff 219103. lebedev.ri marked 4 inline comments as done. lebedev.ri added a comment. Rebased over precommitted NFC changes, much nicer diff! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://re

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. I will still need to see where else `EmitCheckedInBoundsGEP()` should be used, but i'm wondering if it should be best done in follow-ups, since this already catches the interesting bits.. In D67122#1659721 , @vsk wrote: > Sti

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

lebedev.ri accepted this revision. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Okay, thank you for the patience, this looks good to me now too. Please update patch description/subject - this patch is not a standalone tool. Comment at: clang/test/Pro

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

lebedev.ri added inline comments. Comment at: clang/test/Profile/misexpect-branch-cold.c:4 +// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata +// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -v

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

lebedev.ri added a comment. In D67122#1663619 , @vsk wrote: > > In D67122#1659721 , @vsk wrote: > > > >> Still think this looks good. Have you tried running this on the llvm test > >> suite, or some other interest

[PATCH] D33365: [clang-tidy] misc-assertion-count: A New Check

lebedev.ri added a comment. In https://reviews.llvm.org/D33365#802884, @lebedev.ri wrote: > In https://reviews.llvm.org/D33365#775916, @alexfh wrote: > > > I think, this kind of a check needs some prior research (not necessarily in > > the sense of a printed paper, but at least a thoughtful anal

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

lebedev.ri created this revision. lebedev.ri added a project: clang-tools-extra. Herald added subscribers: xazax.hun, JDevlieghere. Currently, there is two configured prefixes: `CHECK-FIXES` and `CHECK-MESSAGES` `CHECK-MESSAGES` checks that there are no test output lines with `warning:|error:`, w

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

lebedev.ri updated this revision to Diff 111731. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. Address review notes: - `constexpr const` -> `constexpr` - moved `test/clang-tidy/check_clang_tidy.py` changes into https://reviews.llvm.org/D36892 Repository: rL LLVM h

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#845417, @JonasToth wrote: > Did I run it over clang or athletic like that? I would be interested in an > overall output. No, not yet. But if it is to be enabled for LLVM repos by default, then the `Threshold` will most definitivel

[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

lebedev.ri added a comment. Please add the following test: (and make sure that it does the right thing :)) bool f_with_preproc_condition() { auto test = 42; assert(test == 42); return test; } I.e. if `-DNDEBUG` is present, variable is not needed, but if `-DNDEBUG` is *NOT* prese

[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

lebedev.ri added a comment. In https://reviews.llvm.org/D37014#850064, @tbourvon wrote: > In https://reviews.llvm.org/D37014#849157, @lebedev.ri wrote: > > > Please add the following test: (and make sure that it does the right thing > > :)) > > > > bool f_with_preproc_condition() { > > aut

[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

lebedev.ri added inline comments. Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:376 +// expression wouldn't really benefit readability. Therefore we abort. +if (NewReturnLength > MaximumLineLength) { + return; Is there really

[PATCH] D71980: [clang-tidy] Fix bug - 44229, 33203 and 32204

lebedev.ri added a comment. Missing tests; please upload patches with full context (`-U9`); just referencing bug# is non-informative - best to say `"[clang-tidy] check-name: fix 'constexpr if' handling (PR#)"`; possibly you want to split this up into per-check patches Repository: rG LLV

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

lebedev.ri added a comment. Thinking about it more, i have some negative reservations about this :/ As it can be seen form https://godbolt.org/z/3BZmCM, it seems any and every(?) alternative C++ algorithm replacement is a performance pessimization in the general case, because `memcpy` requires/

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14322 + if (SrcForMask->getType()->isPointerTy()) { +/// TODO: Use ptrmask instead of ptrtoint/inttoptr +// Result = Builder.CreateIntrinsic( Until we are there, can we still e

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added a comment. In D71499#1800636 , @arichardson wrote: > Address feedback: Avoid inttoptr by using ptrtoint + GEP instead. > > Using two GEPs for align_up seems to generate better code than select + a > single GEP. > See https://godbolt.org/

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14318 + } + // Negate the mask to only clear the lower bits. + llvm::Value *Result; But this isn't what we are doing, negation != inversion. Repository: rG LLVM Github Monorepo

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri accepted this revision. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. This revision is now accepted and ready to land. Looks ok to me now in principle. I have one more question about pointer variants though (see inline) Comment at: clang/li

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added reviewers: nlopes, aqjune. lebedev.ri added a subscriber: nlopes. lebedev.ri added a comment. (would be good for @nlopes to comment, maybe i'm overweighting this..) In D71499#1801119 , @arichardson wrote: > In D71499#1801104

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. (temporarily undoing my review since it looks like things may change) In D71499#1801302 , @arichardson wrote: > ... Aha! :) In D71

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added a comment. Hmm, i keep coming up with things here, sorry :/ I think we should take one more step - explicitly call out that the result of `__builtin_align_*` is, well, aligned :) For that, we need to add `__attribute__((alloc_align(2)))` attribute. Despite the name, it does not

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added a comment. For future reference, if anyone needs here's the C versions of these builtins: https://godbolt.org/z/oHeAh- ^ looks like we are missing some backend-level folds for round-down variant, filed https://bugs.llvm.org/show_bug.cgi?id=8 Repository: rG LLVM Github Mo

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added a comment. In D71499#1802134 , @lebedev.ri wrote: > For future reference, if anyone needs here's the C versions of these builtins: > https://godbolt.org/z/oHeAh- > > ^ looks like we are missing some backend-level folds for round-down vari

[PATCH] D72217: [clang-tidy] Added readability-qualified-auto check

lebedev.ri added a comment. Oh, thank you for working on this one, i've always missed this particular check. Given that there's finally progress on const-correctness check, should this only handle adding `*`/`&`, and leave `const` alone? In D72217#1804628

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri accepted this revision. lebedev.ri added a subscriber: erichkeane. lebedev.ri added a comment. This revision is now accepted and ready to land. Nice. Thank you for working on this. I think this finally fully looks good to me. Not sure whether we should be really adding an attribute to

[PATCH] D72239: [clang-tidy] new opencl recursion not supported check

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. I was just going to look into this kind of check. This can be implemented without recursion in the check itself. Let me write it and post my variant. Repository: rCTE Clang

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

lebedev.ri created this revision. lebedev.ri added reviewers: JonasToth, aaron.ballman, ffrankies, Eugene.Zelenko, erichkeane, NoQ. lebedev.ri added a project: LLVM. Herald added subscribers: xazax.hun, Anastasia, mgorny. Herald added a project: clang. Recursion is a powerful tool, but like any t

[PATCH] D72239: [clang-tidy] new opencl recursion not supported check

lebedev.ri added a comment. In D72239#1805281 , @lebedev.ri wrote: > I was just going to look into this kind of check. > This can be implemented without recursion in the check itself. > Let me write it and post my variant. D72362

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

lebedev.ri added a comment. In D72362#1808825 , @Eugene.Zelenko wrote: > It'll be reasonable to add CERT alias. I'm not sure about that. This diagnoses **any** potential recursion, while CERT is much more specific than that. (`Avoid cycles during initia

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

lebedev.ri updated this revision to Diff 236843. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. s/1/true/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72362/new/ https://reviews.llvm.org/D72362 Files: clang-tools-extra

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

lebedev.ri added a comment. Land this? (branching is near..) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71499/new/ https://reviews.llvm.org/D71499 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D68115: Zero initialize padding in unions

lebedev.ri added a comment. Can patch description be made a bit more verbose? > However with -ftrivial-auto-var-init=pattern those undefs became 0xAA pattern > and break some code. Break how? Does this have to be an unilateral change, likely penalizing non-`-ftrivial-auto-var-init=` cases, i.e

[PATCH] D68115: Zero initialize padding in unions

lebedev.ri added a subscriber: aaron.ballman. lebedev.ri added a comment. In D68115#1811089 , @hubert.reinterpretcast wrote: > In D68115#1810891 , @lebedev.ri > wrote: > > > Does this have to be an unilateral chan

[PATCH] D72438: [clang-tidy] For checker `readability-misleading-indentation` update tests.

lebedev.ri added subscribers: Tyker, lebedev.ri. lebedev.ri added a comment. Hm, didn't clang gain such a diagnostic itself recently? https://godbolt.org/z/MYJTvw Wouldn't it make sense to migrate everything into it, and drop this now-duplicating check? Repository: rG LLVM Github Monorepo C

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

lebedev.ri marked 8 inline comments as done. lebedev.ri added a comment. Thanks for taking a look. Some deflective replies inline. Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:21 + +inline bool operator==(const CallGraphNode::CallRecord &LHS, +

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

lebedev.ri added a comment. A little bit late to the party, but still. I'm honestly wondering if this should be a proper clang static analyzer data-flow aware check. This is basically diagnosing every `signed char` -> `signed int` promotion, regardless of whether the `char` is used as an ASCII

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

lebedev.ri added a comment. I agree that the current alias situation is not ideal, though i'm not sure how much we can fix it since it crosses user interface boundary (i.e. what fixes won't lead to some regressions from the previous expected behavior) To be noted, this will make adding of new al

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

lebedev.ri added a comment. In D72566#1815913 , @njames93 wrote: > In D72566#1815904 , @lebedev.ri > wrote: > > > I agree that the current alias situation is not ideal, though i'm not sure > > how much we can fix

[PATCH] D72566: [clang-tidy] Clang tidy is now alias aware

lebedev.ri added a comment. I think the fact that this is a fourth (?) different incarnation of a patch trying to solve the same *major* *ugly* problem, it may be an evidence that perhaps this problem should not be approached from the 'let's hack it together' approach, but with a concrete plan sen

[PATCH] D72484: [clang-tidy] Fix check for Abseil internal namespace access

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/abseil/NoInternalDependenciesCheck.cpp:43-44 + + if (!LocAtFault.isValid()) +return; + Is there test coverage for this? When does/can this happen? CHANGES SINCE LAST ACTION https

[PATCH] D72362: [clang-tidy] misc-no-recursion: a new check

lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. In D72362#1817599 , @bader wrote: > Does it make sense to implement such diagnostics in clang Sema, considering > that OpenCL does not allow recursion? > We implemented similar diag

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

lebedev.ri added a comment. Happy to see this check! Comment at: clang-tools-extra/docs/ReleaseNotes.rst:199-202 +- New alias :doc:`performance-prefer-pre-increment + ` to + :doc:`llvm-prefer-pre-increment + ` was added. Are we **really** **really** sure thi

[PATCH] D72553: [clang-tidy] Add llvm-prefer-preincrement check

lebedev.ri added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:199-202 +- New alias :doc:`performance-prefer-pre-increment + ` to + :doc:`llvm-prefer-pre-increment + ` was added. njames93 wrote: > lebedev.ri wrote: > > Are we **really**

[PATCH] D72553: [clang-tidy] Add performance-prefer-preincrement check

lebedev.ri added a comment. In D72553#1820692 , @njames93 wrote: > - moved check from llvm module to performance Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72553/new/ https://reviews.llvm.or

[PATCH] D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI.

lebedev.ri added a comment. > Sign-extension is not guaranteed by the ABI, and thus the callee cannot > assume it. This is a bit too vague. Can you quote specific parts of the appropriate standard/document? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`

lebedev.ri added a comment. Thank you for looking into it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87527/new/ https://reviews.llvm.org/D87527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D71199: [clang-tidy] New check cppcoreguidelines-prefer-member-initializer

lebedev.ri added a comment. Thank you for looking into it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71199/new/ https://reviews.llvm.org/D71199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D87825: [ASTMatchers] Define clang::ast_matchers::decompositionDecl

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. SGTM, that's a pretty traditional issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87825/new/ https://reviews.llvm.org/D87825 _

[PATCH] D87972: [OldPM] Pass manager: run SROA after (simple) loop unrolling

lebedev.ri updated this revision to Diff 292984. lebedev.ri added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fixing a few clang tests and updating one more llvm test to check this also. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D87972: [OldPM] Pass manager: run SROA after (simple) loop unrolling

lebedev.ri added a comment. (I'm guessing that we are talking about run-time performance here.) In D87972#2284060 , @MaskRay wrote: > I have tested this patch internally and seen gains and losses. On one > document search related benchmark 3~5% improveme

[PATCH] D87972: [OldPM] Pass manager: run SROA after (simple) loop unrolling

lebedev.ri added a comment. In D87972#2284064 , @xbolva00 wrote: > In D87972#2284060 , @MaskRay wrote: > >> I have tested this patch internally and seen gains and losses. On one >> document search related benchmark

[PATCH] D71199: [clang-tidy] New check cppcoreguidelines-prefer-member-initializer

lebedev.ri resigned from this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. In D71199#2285076 , @baloghadamsoftware wrote: > Prerequeisite patch is committed, the check is tested now on the //LLVM > Project//. @le

[PATCH] D87972: [OldPM] Pass manager: run SROA after (simple) loop unrolling

lebedev.ri added a comment. In D87972#2285176 , @arsenm wrote: > I assume this makes 1f4e7463b5e3ff654c84371527767830e51db10d > > redundant? Yes, see `llvm/lib/Target/AMDGPU/AMDGPUTar

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

lebedev.ri updated this revision to Diff 293678. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Rebased. There is a number of official open-source LGPL-3 implementations already: - https://github.com/SonarSource/SonarTS/pull/378 - https://github.com/SonarSource/sonar

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

lebedev.ri added a comment. In D36836#2292541 , @aaron.ballman wrote: > In D36836#2289639 , @lebedev.ri > wrote: > >> Rebased. >> >> There is a number of official open-source LGPL-3 implementations already: >> >>

[PATCH] D84176: [CMake][CTE] Add "check-clang-extra-..." targets to test only a particular Clang extra tool

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. LGTM, please feel free to land this as-is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84176/new/ https://reviews.llvm.org/D84176 __

[PATCH] D87972: [OldPM] Pass manager: run SROA after (simple) loop unrolling

lebedev.ri added a comment. @MaskRay, @dmgreen & @sanwou01 thank you for running perf experiment! I think all the results are consistent along the lines of "this sounds generally reasonable (esp. given that new-pm does it already), as usual results in ups&downs, but seems to be a (small) geomean

[PATCH] D87972: [OldPM] Pass manager: run SROA after (simple) loop unrolling

lebedev.ri added a comment. In D87972#2294488 , @xbolva00 wrote: >>> Does that sound reasonable? > > Yes IMHO. > >>> What are the next suggested steps? > > It would be great to isolate and check the cases which regressed a bit. I've rerun my benchmark, an

[PATCH] D86369: [Sema][MSVC] warn at dynamic_cast when /GR- is given

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/ms_dynamic_cast.cpp:16 +B *b = new D1(); +auto d = dynamic_cast(b); // expected-warning{{should not use dynamic_cast with /GR-}} +} I'm not sure it makes sense to talk about MSVC/clang-cl f

[PATCH] D85097: [Sema] add warning for comparisons like 'x<=y<=z'

lebedev.ri added a comment. In D85097#2201609 , @vabridgers wrote: > Thanks for the recent comments. I just pushed a few improvements over the > last patch that didn't comprehend latest comments from @rsmith and > @Quuxplusone. I'll read through those ca

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added a comment. Release notes missing. I think the canonical way to silence it (via masking?) should be at least mentioned. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884 llvm::Value *BitsShiftedOff = Builder.CreateLShr( Ops.LHS, Builder

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

lebedev.ri added a comment. This really needs some docs changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + Surely not `~1U`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/ne

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + vsk wrote: > jfb wrote: > > lebedev.ri wrote: > > > Surely not `~1U`. > > Indeed, fixed. > I don't think this pattern wor

[PATCH] D86841: [clang] Adds noprogress attribute deduction for infinite loops

lebedev.ri added a comment. Does that wording apply to both the C and C++ (OpenCL?)? Regardless, might be good to test that. Comment at: clang/test/CodeGen/attr-noprogress.c:1-2 +// RUN: %clang_cc1 -S -emit-llvm %s -o - | grep " noprogress " | count 1 +// RUN: %clang_cc1 -S -em

[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

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

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. I feel like this may be trying to do too many things at once. Was there an RFC? There are several patches here: 1. langref 2. llvm side of the patch (maybe should be in the pr

<    10   11   12   13   14   15   16   17   18   19   >