[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

lebedev.ri added a comment. In D64695#1585772 , @Manikishan wrote: > In D64695#1585754 , @rdwampler wrote: > > > I am not quite sure why this change is required to sort the headers for > > NetBSD, you can set the p

[PATCH] D64644: Fixes an assertion failure while instantiation a template with an incomplete typo corrected type

lebedev.ri added inline comments. Comment at: clang/test/SemaTemplate/instantiate-incomplete-typo-suggested-error-limit.cpp:9-10 + +#include +#include + Please don't pull in any external headers - just mock what you need. CHANGES SINCE LAST ACTION https://

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

lebedev.ri added reviewers: JonasToth, gribozavr. lebedev.ri added a comment. Thanks. Are there any tests missing for `volatile`, atomics? I'm not really current on clang-tidy state of affairs, so i'm gonna leave most of the review for others.. Comment at: clang-tidy/bugprone/

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

lebedev.ri added a comment. Is there sufficient test coverage as to what happens if `SortPriority` is not set? Comment at: lib/Format/Format.cpp:455-456 Style.KeepEmptyLinesAtTheStartOfBlocks); -IO.mapOptional("BitFieldDeclarationsOnePerLine", Style.Bi

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

lebedev.ri added a comment. In D64695#1589740 , @Manikishan wrote: > In D64695#1589508 , @lebedev.ri > wrote: > > > Is there sufficient test coverage as to what happens if `SortPriority` is > > not set? > > > If S

[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables

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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

lebedev.ri added a comment. Just passing-by thought: These attributes that are being added implicitly, it will be possible to differentiate whether an attribute was actually present in the code, or was added implicitly, say for clang-tidy check purposes? Is there some 'implicit' bit on these, or

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

lebedev.ri added a comment. In D64448#1595022 , @mgehre wrote: > In D64448#1595017 , @lebedev.ri > wrote: > > > Just passing-by thought: > > These attributes that are being added implicitly, it will be possible to

[PATCH] D64644: Fixes an assertion failure while instantiation a template with an incomplete typo corrected type

lebedev.ri added reviewers: aaron.ballman, erichkeane. lebedev.ri added a comment. Test looks good, adding a few more potential reviewers.. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64644/new/ https://reviews.llvm.org/D64644 ___ cfe-com

[PATCH] D65202: [Support] Fix `-ftime-trace-granularity` option

lebedev.ri added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:1943-1947 .. option:: -ftime-report +.. option:: -ftime-trace + +Turn on time profiler While there add a description to `-ftime-report` and document how they are different?

[PATCH] D65202: [Support] Fix `-ftime-trace-granularity` option

lebedev.ri added a comment. Looks ok Comment at: clang/docs/ClangCommandLineReference.rst:1943-1947 .. option:: -ftime-report +.. option:: -ftime-trace + +Turn on time profiler lebedev.ri wrote: > While there add a description to `-ftime-report` and document

[PATCH] D65300: [clang] [CodeGen] clang-misexpect prototype for compiler warnings

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. Thank you for working on this! I'm guessing this doesn't have a `-Werror=` mode? I still believe this should output a remark. It will still be visible in the compiler console

[PATCH] D65556: Phabricator D49466

lebedev.ri added a comment. Please fix patch title and description Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65556/new/ https://reviews.llvm.org/D65556 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D65589: [clang] Fix mismatched args constructing AddressSpaceAttr.

lebedev.ri added a comment. This really needs a test, likely an astdump-one will show it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65589/new/ https://reviews.llvm.org/D65589 ___ cfe-commits mailing list cfe-comm

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

lebedev.ri added inline comments. Comment at: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35 + // This may work fine when optimization is enabled because bar() can + // be turned into a constant 7. But without optimization, it can + // cause

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

lebedev.ri added a comment. I think this last update clarified the concerns i raised in last comment. But, did you mean to update the tests? They should have broke now that you require `!getLangOpts().ThreadsafeStatics`. Comment at: clang-tools-extra/docs/clang-tidy/checks/bu

[PATCH] D65706: [docs] document -Weveything more betterer

lebedev.ri added inline comments. Comment at: clang/docs/UsersManual.rst:998 +:option:`-Werror`, and also includes the warnings from :option:`-pedantic`. Some +diagnostics contradict each other, users :option:`-Weverything` therefore often +disable many diagnostics such as :opti

[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:47 +void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics) +return

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

lebedev.ri added a comment. In D65065#1617031 , @gribozavr wrote: > > This suggestion would result another strange behavior: if the user disables > > cert-err09-cpp because he or she doesn't want to see its reports, the other > > one (cert-err61-cpp) wil

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

lebedev.ri added a comment. In D65065#1617079 , @gribozavr wrote: > `-Weverything` is not recommended for anything except compiler testing, and > similarly with clang-tidy, enabling all checkers is not a good idea. I don't > think improving this particul

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

lebedev.ri added a comment. In D62731#1619892 , @mibintc wrote: > Compared to the 2nd revision, this patch moves all the changes into clang, > removing the FPState file. > > In the summary, I've copied information from Microsoft about the fp-model > opti

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

lebedev.ri added a comment. Why do you need to change (update) the datalayout in every single test? That looks extremely noisy and hides the actually-needed changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64931/new/ https://reviews.llvm.org/

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

lebedev.ri added inline comments. Comment at: clang/lib/Basic/Targets/OSTargets.h:771-772 } else if (Triple.getArch() == llvm::Triple::x86) { - this->resetDataLayout("e-m:e-p:32:32-i64:64-n8:16:32-S128"); + this->resetDataLayout("e-m:e-p:32:32-p253:32:32-p254:32:32

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

lebedev.ri added a reviewer: reames. lebedev.ri added inline comments. Comment at: llvm/lib/Target/X86/X86TargetMachine.cpp:119-120 + // Address spaces for 32 bit signed, 32 bit unsigned, and 64 bit pointers. + Ret += "-p253:32:32-p254:32:32-p255:64:64"; + I

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

lebedev.ri added a comment. In D64931#1622038 , @akhuang wrote: > @lebedev.ri The test case datalayout strings were changed because somewhere > llvm asserts that the string in the IR matches the backend datalayout. I > don't know why I wasn't getting the

[PATCH] D64931: Change X86 datalayout for three address spaces that specify pointer sizes.

lebedev.ri added a comment. In D64931#1622144 , @akhuang wrote: > > Can you post a reproducer? > > Turns out I just didn't have assertions enabled. With assertions the changed > test cases should fail. > > > I think this is precisely what was discussed in

[PATCH] D66143: Don't use std::errc

lebedev.ri added a comment. In D66143#1627221 , @ABataev wrote: > In D66143#1627195 , @thakis wrote: > > > As far as I know 4.8 is no longer supported. > > > > In D66143#1627168

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

lebedev.ri added a comment. LGTM Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D66254: Correct include suggestion when search path includes symlink

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

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri added a comment. In D66186#1631921 , @Nathan-Huckleberry wrote: > In D66186#1630427 , @aaron.ballman > wrote: > > > There was a request in the linked bug for some code archaeology to see why > > this b

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. (just want to mark it as "unanswered questions") Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66186/new/ https://reviews.llvm

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

lebedev.ri added reviewers: lebedev.ri, vsk. lebedev.ri added a comment. This is marked as child revision of D65300 but it seems like they both add the same logic, just into different components, D65300 to clang, this to llvm. I

[PATCH] D65997: Add options rounding and exceptions to pragma fp

lebedev.ri added a comment. Since LLVM already has strict FP nodes, i'd personally like to see this RFC to not discuss "let's add XYZ", but discuss what features it targets to accomplish, what features are already achievable via strict FP nodes, and instead of adding a third set of FP operations,

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

lebedev.ri added a comment. In D64943#1633164 , @ABataev wrote: > In D64943#1619958 , @sdmitriev wrote: > > > As I understand ‘atexit’ solution would be target dependent (‘__cxa_atexit’ > > on Linux and ‘atexit’ on

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

lebedev.ri added a comment. In D64943#1633357 , @vzakhari wrote: > In D64943#1633175 , @ABataev wrote: > > > In D64943#1633170 , @lebedev.ri > > wrote: > > > > > In D64943#1

[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

lebedev.ri added a comment. Why is this in android module? Is that android-specific behavior, or posix? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63623/new/ https://reviews.llvm.org/D63623 ___ cfe-

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

lebedev.ri added a comment. In D63625#1552756 , @akhuang wrote: > Looks good, alternatively I think we can just change `%result.ptr` into a > variable match? Why isn't this using FileCheck variable matching in the first place? That's the least fragile

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

lebedev.ri resigned from this revision. lebedev.ri added a comment. I'm sorry, i should have noticed that earlier, the handling of `"Frontend"` section still looks way too intrusive to me, it breaks abstractions. The only simple solution i see is to 1. Insert a timer for `PrettyStackTraceStrin

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

lebedev.ri added a comment. Hm, i started writing previous comment before you posted your last comment, so i didn't see the last update. This looks less intrusive, yes, but two observations: 1. You now have two `"Frontend"` sections - first one being for lexing time 2. That lexing section is not

[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

lebedev.ri added a comment. In D63623#1552716 , @jcai19 wrote: > In D63623#1552679 , @lebedev.ri > wrote: > > > Why is this in android module? > > Is that android-specific behavior, or posix? > > > I implemented i

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

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. In D62977#1540184 , @lebedev.ri wrote: > Without seeing the tests - what version checks does this have? > It shouldn't fire if the go

[PATCH] D63774: android: enable double-word CAS on x86_64

lebedev.ri added a comment. Is there some test for this area of code? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63774/new/ https://reviews.llvm.org/D63774 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

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

lebedev.ri added a comment. It //sounds// correct, but i don't see accompanying test changes, so i can't be sure. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62977/new/ https://reviews.llvm.org/D62977 ___ cfe-commits mailing list cfe-com

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

lebedev.ri added a comment. In D62977#1559637 , @astrelni wrote: > In D62977#1559628 , @lebedev.ri > wrote: > > > It //sounds// correct, but i don't see accompanying test changes, so i > > can't be sure. > > > The

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

lebedev.ri added a comment. In D63048#1559880 , @dexonsmith wrote: > In D63048#1558806 , @sylvestre.ledru > wrote: > > > @dexonsmith ping? > > > ... > FWIW, I feel similarly about `-dumpversion` (better to remo

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

lebedev.ri added a comment. In D63048#1559894 , @lebedev.ri wrote: > In D63048#1559880 , @dexonsmith > wrote: > > > In D63048#1558806 , > > @sylvestre.ledru wrote: > > > >

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

lebedev.ri added a comment. What's the target use-case here? What can't be solved with normal attributes? I wonder if this should go to cfe+llvm -dev lists first, it's kinda intrusive. I also wonder if all these should cause a clang diagnostic, at least under `-Wall`. How is versioning expected t

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

lebedev.ri resigned from this revision. lebedev.ri added a comment. Looks reasonable. I did not review the check itself though. Are `test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp` and `test/clang-tidy/google-upgrade-googletest-case.cpp ` identical other than the included header and

[PATCH] D63638: [clang][NewPM] Add new pass manager RUN lines to avx512f-builtins.c

lebedev.ri added a comment. In D63638#1560846 , @spatel wrote: > I skimmed D63174 but haven't applied either > of these patches to test locally, so I may not have the full picture. > > IMO, we do not want clang regression

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

lebedev.ri added a comment. In D63845#1561793 , @jdoerfert wrote: > In D63845#1560605 , @aaron.ballman > wrote: > > > In D63845#1559995 , @lebedev.ri > > wrote: > > > > > W

[PATCH] D62855: [clangd] Implementation of auto type expansion.

lebedev.ri added a comment. The patch is completely missing description. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62855/new/ https://reviews.llvm.org/D62855 ___ cfe-commits mailing list cfe-commit

[PATCH] D63929: [clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

lebedev.ri added a comment. I have the same observation as with googletest patch (D62977 ), and i'd guess this applies to all these clang-tidy abseil checks (meaning the existing ones are all equally broken): there is no checking that the new macro actually exist

[PATCH] D63929: [clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

lebedev.ri added a comment. (Stuck reviews are bad, but blind stamping is even worse..) Comment at: clang-tools-extra/clang-tidy/abseil/PrefixedThreadAnnotationsCheck.cpp:15-16 + +constexpr char kAbseilThreadAnnotationsHeader[] = +"absl/base/internal/thread_annotations.h";

[PATCH] D63929: [clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

lebedev.ri requested changes to this revision. lebedev.ri added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/clang-tidy/abseil/PrefixedThreadAnnotationsCheck.cpp:41 + llvm::StringRef(kAbseilThreadAnnotationsHeader); + if (ac

[PATCH] D62962: Clang implementation of sizeless types

lebedev.ri added a comment. Since this is an extension, it wouldb be great to have a (on-by-default? at least in `-Wall`) diagnostic for every instance of usage of this extension. (I guess this is actually true for clang vector extension, too..) Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

lebedev.ri added a comment. To make this transformation safe, I have changed clang's code-generation to always load virtual function pointers using the llvm.type.checked.load intrinsic, instead of regular load instructions. I originally tried writing this using clang's existing code-genera

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp:42-43 + const ASTContext &Context) { + assert(Literal->getLength() == 1); + assert(Literal->getCharByteWidth() ==

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

lebedev.ri added a comment. Nice, thank you for working on this! It would be also good to have a not OpenMP-5 specific, but an overview table - which OpenMP thingy was supported starting with which Clang version, very much like https://clang.llvm.org/cxx_status.html / https://libcxx.llvm.org/cx

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

lebedev.ri added inline comments. Comment at: clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp:55 + + absl::StrSplit("ABC", R"(A)"); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() called with a string literal consisting of a single charac

[PATCH] D64151: Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp:55 + + absl::StrSplit("ABC", R"(A)"); + // CHECK-MESSAGES: [[@LINE-1]]:25: warning: absl::StrSplit() called with a

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

lebedev.ri added a comment. In D64375#1577153 , @jdoerfert wrote: > @lebedev.ri I'd like to see if we can transition this one into a more generic > one with version numbers etc. Is that OK? I have no further comment at the moment. Repository: rG LLV

[PATCH] D61001: [clang-format][tests] Explicitly specify style in some tests

lebedev.ri added inline comments. Comment at: test/Format/language-detection.cpp:2 // RUN: grep -Ev "// *[A-Z0-9_]+:" %s \ -// RUN: | clang-format -style=llvm -assume-filename=foo.js \ // RUN: | FileCheck -strict-whitespace -check-prefix=CHECK1 %s What's wr

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

lebedev.ri added a comment. In D64454#1579336 , @Eugene.Zelenko wrote: > May be script should generate documentation during build, so files .rst files > are not needed? I'd personally weakly advise against that, to be honest; that would not be inherent

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

lebedev.ri requested changes to this revision. lebedev.ri added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:270 + AllowPointerConditions(Options.get("AllowPointerConditi

[PATCH] D61749: [clang-tidy] initial version of readability-const-method

lebedev.ri resigned from this revision. lebedev.ri added a comment. Sorry, it does not appear that i'm being useful in this review. Comment at: clang-tools-extra/clang-tidy/readability/StaticMethodCheck.cpp:89-101 + isDefinition(), isUserProvided(), unless(isExpansio

[PATCH] D64543: [Docs] Add standardized header links to analyzer doc

lebedev.ri added a comment. I like this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64543/new/ https://reviews.llvm.org/D64543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D61749: [clang-tidy] initial version of readability-static-const-method

lebedev.ri added a reviewer: lebedev.ri. lebedev.ri added a comment. In D61749#1497162 , @Eugene.Zelenko wrote: > @JonasToth tried to implement const check, so probably const and static part > should be split. I agree, there is D54943

[PATCH] D61749: [clang-tidy] initial version of readability-static-const-method

lebedev.ri added a comment. In D61749#1498541 , @mgehre wrote: > ... Still, this is two separate checks, logically. You can put the `FindUsageOfThis` e.g. into a new file in `clang-tidy/utils`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D61809: [BPF] Preserve original struct/union type name/access index and array subscripts

lebedev.ri added a comment. Tests seems to be missing? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61809/new/ https://reviews.llvm.org/D61809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

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

lebedev.ri added a comment. This will now trigger on https://godbolt.org/z/9oFMcB right? Just want to point out that this will then have "false-positives" when that loop is an OpenMP for loop, since range-for loop is not available until OpenMP 5. I don't think this false-positive can be avoided t

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

lebedev.ri added a comment. In D61827#1499183 , @hintonda wrote: > In D61827#1499160 , @lebedev.ri > wrote: > > > This will now trigger on https://godbolt.org/z/9oFMcB right? > > Just want to point out that this w

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

lebedev.ri added a comment. In D61827#1499303 , @torbjoernk wrote: > In D61827#1499184 , @lebedev.ri > wrote: > > > In D61827#1499183 , @hintonda > > wrote: > > > > > In D6

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

lebedev.ri added a comment. In D61827#1499306 , @hintonda wrote: > In D61827#1499303 , @torbjoernk > wrote: > > > In D61827#1499184 , @lebedev.ri > > wrote: > > > > > In D6

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

lebedev.ri added a comment. In D61827#1499333 , @hintonda wrote: > In D61827#1499309 , @lebedev.ri > wrote: > > > In D61827#1499306 , @hintonda > > wrote: > > > > > In D618

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

lebedev.ri added a comment. In D61827#1499347 , @hintonda wrote: > In D61827#1499335 , @lebedev.ri > wrote: > > > In D61827#1499333 , @hintonda > > wrote: > > > > > When I

[PATCH] D24892: [clang-tidy] Add option "LiteralInitializers" to cppcoreguidelines-pro-type-member-init

lebedev.ri added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:66-69 +- Added `UseAssignment` option to `cppcoreguidelines-pro-type-member-init` + + If set to true, the check will provide fix-its with literal initializers + (``int i = 0;``) instead of cur

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

lebedev.ri added a comment. Sounds good, thank you! Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst:262 +not been used on code with a compatibility requirements of OpenMP prior to +version 5. I would add a cross-reference to `NO

[PATCH] D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference

lebedev.ri added inline comments. Comment at: docs/ReleaseNotes.rst:182-184 +- New options `WarnOnLargeObject` and `MaxSize` for check + :doc:`misc-throw-by-value-catch-by-reference + ` check to warn for check :doc:`..`<..> check (repeated 'check' word) Repo

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

lebedev.ri added inline comments. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") { } -void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer, +void HandlePragma(Preprocessor &PP, PragmaIntroducer Intro

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

lebedev.ri added inline comments. Comment at: lib/Frontend/HeaderIncludeGen.cpp:55 + // Simplify Filename that starts with "./" + if (Filename.startswith("./")); +Filename=Filename.substr(2); xiangzhangllvm wrote: > Need remove ";" ? This was fixed but no

[PATCH] D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference

lebedev.ri added a comment. Looks good Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h:45 + const bool WarnOnLargeObject; + uint64_t MaxSize; // No `const` bacause we have to set it in two steps. }; because CHANGES SINCE LAST ACTION https

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

lebedev.ri added inline comments. Comment at: lib/Frontend/HeaderIncludeGen.cpp:55 + // Simplify Filename that starts with "./" + if (Filename.startswith("./")); +Filename=Filename.substr(2); skan wrote: > craig.topper wrote: > > skan wrote: > > > lebedev.r

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

lebedev.ri added inline comments. Comment at: lib/Frontend/HeaderIncludeGen.cpp:55 + // Simplify Filename that starts with "./" + if (Filename.startswith("./")); +Filename=Filename.substr(2); skan wrote: > lebedev.ri wrote: > > skan wrote: > > > craig.toppe

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

lebedev.ri accepted this revision. lebedev.ri added a comment. No one raised any concerns, so let's do **this**. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") { } -void HandlePragma(Preprocessor &PP, PragmaIntroduce

[PATCH] D61939: AArch64: add support for arm64_23 (ILP32) IR generation

lebedev.ri added a comment. The subject has a typo, 23 instead of 32 ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61939/new/ https://reviews.llvm.org/D61939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D61809: [BPF] Preserve debuginfo array/union/struct type/access index

lebedev.ri resigned from this revision. lebedev.ri added a comment. Don't think i will be of any help here. Comment at: test/CodeGen/bpf-offsetreloc.c:2-4 +// RUN: grep "llvm.preserve.struct.access.index" %t1 +// RUN: grep "llvm.preserve.array.access.index" %t1 +// RUN: grep "l

[PATCH] D61912: [analyzer] print() JSONify: Store implementation

lebedev.ri added a comment. All these patches bypassed cfe-commits. Why does this invent a yet another json formatter instead of using `"llvm/Support/JSON.h"`, in particular it's lightweight `json::OStream` ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61912/ne

[PATCH] D61912: [analyzer] print() JSONify: Store implementation

lebedev.ri added a comment. In D61912#1521352 , @Charusso wrote: > Hey @lebedev.ri, thanks for the review! > > In D61912#1521306 , @lebedev.ri > wrote: > > > All these patches bypassed cfe-commits. > > > Bypassed?

[PATCH] D55595: Add missing bugprone checks to clang-tidy plugin

lebedev.ri added a comment. 1. Please always upload all patches with full context. 2. There are two places where this pattern exists - this file, and `tool/ClangTidyMain.cpp`. It clearly leads to such issues, Can this be reworked to be just one file that is simply included in both places? Repo

[PATCH] D55595: Share the forced linking code between clang-tidy tool and plugin

lebedev.ri added inline comments. Comment at: clang-tidy/ClangTidyForceLinker.h:1 +#include "llvm/Support/Compiler.h" + Standard prologue missing CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55595/new/ https://reviews.llvm.org/D55595 _

[PATCH] D55595: Share the forced linking code between clang-tidy tool and plugin

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Thanks, LG. You probably may want to wait a bit (a day?) in case others want to comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55595/new/ https://reviews.llvm.org/D555

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

lebedev.ri requested changes to this revision. lebedev.ri added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/abseil/MakeUniqueCheck.h:29 +/// \endcode +class MakeUniqueCheck : public modernize::MakeSmartPtrCheck { +public: ---

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

lebedev.ri added a comment. In D55044#1329604 , @hokein wrote: > In fact, the existing `modernize-make-unique` can be configured to support > `absl::make_unique`, you'd just need to configure the check option > `MakeSmartPtrFunction` to `absl::make_uniqu

[PATCH] D55595: [clang-tidy] Share the forced linking code between clang-tidy tool and plugin

lebedev.ri added inline comments. Comment at: clang-tidy/plugin/ClangTidyPlugin.cpp:11 #include "../ClangTidy.h" +#include "../ClangTidyForceLinker.h" #include "../ClangTidyModule.h" yvvan wrote: > It seems it had to go after #include "clang/Config/config.h" to

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

lebedev.ri added a comment. Clang reviews should mainly go to cfe-commits, not llvm-commits. (all that will happen automagically if one sets the correct repo for the differential..) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54176/new/ https://reviews.llvm.or

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

lebedev.ri added a comment. Please add tests with preprocessor (`#if ...`) that will show that it ignores disabled code. e.g.: class ProbablyValid { private: int a; #if defined(ZZ) public: int b; #endif private: int c; protected: int d; public: int e; }; C

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

lebedev.ri added a comment. In D55793#1335249 , @m4tx wrote: > In D55793#1333661 , @lebedev.ri > wrote: > > > Please add tests with preprocessor (`#if ...`) that will show that it > > ignores disabled code. e.g.:

[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

lebedev.ri updated this revision to Diff 179272. lebedev.ri added a comment. Rebased, NFC. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54589/new/ https://reviews.llvm.org/D54589 Files: docs/ReleaseNotes.rst docs/UndefinedBehaviorSanitizer.rst lib/CodeGen

[PATCH] D56025: [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

lebedev.ri added inline comments. Comment at: test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp:2-3 +// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix %t -- \ +// RUN: -config="{CheckOptions: [{key: readability-uppercase-literal-suffix.Ign

[PATCH] D56025: [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. LG i guess. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56025/new/ https://reviews.llvm.org/D56025 ___ cfe-commits mailing lis

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