[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new check

lebedev.ri added a comment. In D57113#1439890 , @aaron.ballman wrote: > LGTM aside from a minor diagnostic wording nit. Thank you for the review! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57113/new/ h

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri added a comment. Might warrant test coverage in `llvm/unittest/ADT/APSIntTest.cpp`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits m

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

lebedev.ri added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1795 + BranchHint getBranchHint() const { return IfStmtBits.Hint; } + `lib/AST/TextNodeDumper.cpp` will need to be taught to print it too (and astdumper test coverage to show that)

[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

lebedev.ri added a comment. In D35068#1441581 , @koldaniel wrote: > Bug fixing: faulty handling of built-in functions. Please open a new differential, that is a completely new patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35068/new/ htt

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp:30 +// out of order designators +A a1 = {.y = 1, .x = 2}; // expected-warning {{designated initializers are a C99 feature}} + hintonda wrote: > Rakete wrote: >

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

lebedev.ri added a reviewer: aaron.ballman. lebedev.ri added a comment. Please always upload all patches with full context (`-U9`) I'm not sure why we want this? What is wrong with simply applying clang-tidy twice? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://re

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

lebedev.ri added a comment. It is best to error-out early. Could you please try this instead: Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:39 } } // namespace ``` AST_MATCHER_P(CXXNewExpr, hasInitializationStyle, CXXNewExpr::Initiali

[PATCH] D59873: Add additional mangling for struct members of non trivial structs

lebedev.ri added a comment. There is no test here. Tests should probably go into `clang/test/CodeGen`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59873/new/ https://reviews.llvm.org/D59873 ___ cfe-c

[PATCH] D59934: Compare SourceLocations from different TUs by FileID

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

[PATCH] D59963: Add a clang-tidy module for the Linux kernel.

lebedev.ri added a comment. Missing `docs/ReleaseNotes.rst` and `docs/clang-tidy/index.rst` changes. Comment at: clang-tools-extra/clang-tidy/linux/LinuxTidyModule.cpp:13 + +using namespace clang::ast_matchers; + You don't need this here. Repository: rG LLV

[PATCH] D60059: [Driver] implement -feverything

lebedev.ri added a comment. In D60059#1450252 , @yuxuanchen1997 wrote: > > enable all runtime sanitizers for extra safety > > Brilliant idea. But I wonder how that could be done for -fsanitize. ASAN and > MSAN can't be used together. Nonsense. There is

[PATCH] D60123: [AST] Forbid copy/move of Stmt.

lebedev.ri added a comment. Probably `Decl` too? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60123/new/ https://reviews.llvm.org/D60123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

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

lebedev.ri resigned from this revision. lebedev.ri added a comment. Hmm, i suppose this looks good to me now. It would be good to early-exit, but i'm not sure how important that is. Please see D52771 / `clang-tools-extra/trunk/clang-tidy/misc/NonPrivateMemberVari

[PATCH] D60326: Fix error in NamedDeclPrinterTest

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. LG Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60326/new/ https://reviews.llvm.org/D60326 ___ cfe-comm

[PATCH] D60335: Use -fomit-frame-pointer when optimizing PowerPC code

lebedev.ri added a comment. test coverage? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60335/new/ https://reviews.llvm.org/D60335 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[PATCH] D60379: Make precompiled headers reproducible

lebedev.ri added a comment. Needs a test. Comment at: lib/Serialization/ASTWriter.cpp:4283 + // Sort to allow reproducible .pch files - https://bugs.debian.org/877359 + std::map> sortedOpenCLTypeExtMap; for (const auto &I : SemaRef.OpenCLTypeExtMap) { Woul

[PATCH] D60055: Check i < FD->getNumParams() before querying

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/PR41139.cpp:3 + +// expected-no-diagnostics + Add a comment what this test does? ('just checking that the crash does not happen again') Comment at: clang/test/SemaCXX/cxx1y-generi

[PATCH] D60055: Check i < FD->getNumParams() before querying

lebedev.ri added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2895 // function and isn't necessarily referring to one of FD's parameters. - if (FD->getParamDecl(i) == PV) + if (i < FD->getNumParams() && FD->getParamDecl(i) == PV)

[PATCH] D60379: Make precompiled headers reproducible

lebedev.ri added inline comments. Comment at: lib/Serialization/ASTWriter.cpp:4283 + // Sort to allow reproducible .pch files - https://bugs.debian.org/877359 + std::map> sortedOpenCLTypeExtMap; for (const auto &I : SemaRef.OpenCLTypeExtMap) { Anastasia wrot

[PATCH] D60363: [clang-format] [PR41170] Break after return type ignored with certain comments positions

lebedev.ri added a comment. Please either subscribe the correct lists or set the correct repo in differential params. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60363/new/ https://reviews.llvm.org/D60363 ___ cfe-

[PATCH] D60055: Check i < FD->getNumParams() before querying

lebedev.ri resigned from this revision. lebedev.ri added a comment. I won't be able to properly review this, sorry. While i'm sure this fix silences the assert, i don't know what that means for the original caller. Does it fail? Does it retry in some other scope? Should it be more picky about th

[PATCH] D60151: [clang-tidy] Rename llvm checkers to llvm-project

lebedev.ri added a comment. In D60151#1462976 , @hintonda wrote: > In D60151#1462850 , @MyDeveloperDay > wrote: > > > are we supporting "-llvm-*" in existing .clang-tidy files? > > > > If people selectively turn ch

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

lebedev.ri added a comment. Some post-commit review (please submit a new review, don't replace this diff) As usual, the incorrect license headers keep slipping through. Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:5-6 +// +// This file is distributed under the Uni

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

lebedev.ri added inline comments. Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:29-30 + +static std::string escapeString(StringRef Src) { + std::string OS; + for (const unsigned char &C : Src) { lebedev.ri wrote: > `SmallString<32>` ? > Also, it is safe t

[PATCH] D60663: Time profiler: small fixes and optimizations

lebedev.ri added a comment. Looks good ignoring the json bits. Re license: > https://reviews.llvm.org/D58675 > This is the first part of time tracing system, I have splitted them cause > this part is mostly written by Aras Pranckevicius except of several minor > fixes concerning formatting.

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

lebedev.ri added a comment. In D58675#1465706 , @anton-afanasyev wrote: > In D58675#1465336 , @lebedev.ri > wrote: > > > Some post-commit review (please submit a new review, don't replace this > > diff) > > As u

[PATCH] D60663: Time profiler: small fixes and optimizations

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Ok, LG, thank you! Comment at: llvm/lib/Support/TimeProfiler.cpp:30 +cl::desc( +"Minimum time granularity (in microsecons) traced by time profiler"), +

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

lebedev.ri added a comment. > Not sure how to test this though? I guess we can trust that std::map is > always sorted just as it says in its description. You could add a test that contains several entries in `OpenCLTypeExtMap` and several entries in `OpenCLDeclExtMap`. In that test, write te PC

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri requested changes to this revision. lebedev.ri added inline comments. This revision now requires changes to proceed. Comment at: llvm/unittests/ADT/APSIntTest.cpp:188-194 + EXPECT_FALSE(CharBoundaryUnder.isNegative()); + EXPECT_TRUE(CharBoundaryUnder.isNonNegative())

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: llvm/unittests/ADT/APSIntTest.cpp:162 +TEST(APSIntTest, UnsignedHighBit) { + APSInt False(APInt(1, 0)); Can you please duplicate this whole test but for signed `APSInt`?

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri added a comment. In D59712#1469295 , @craig.topper wrote: > Wondering if it would be better to assert for asking for the sign of an > unsigned APSInt. I could see a caller just wanting to get the msb for some > reason and not knowing that isN

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri added a comment. In D59712#1469358 , @jdenny wrote: > In D59712#1469301 , @lebedev.ri > wrote: > > > In D59712#1469295 , @craig.topper > > wrote: > > > > > Wonde

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri added a comment. In D59712#1469693 , @jdenny wrote: > In D59712#1469392 , @lebedev.ri > wrote: > > > In D59712#1469358 , @jdenny wrote: > > > > > In D59712#146930

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

lebedev.ri added a comment. In D60778#1470152 , @Anastasia wrote: > In D60778#1468825 , @lebedev.ri > wrote: > > > > Not sure how to test this though? I guess we can trust that std::map is > > > always sorted just

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

lebedev.ri added a comment. In D60778#1470181 , @riccibruno wrote: > Mmm. I hope I am not missing something obvious, but how is this actually > fixing the issue ? I don't see why the order between the `Type *` and between > the `Decl *` should be determi

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri resigned from this revision. lebedev.ri added a comment. In D59712#1472318 , @jdenny wrote: > In D59712#1469760 , @lebedev.ri > wrote: > > > In D59712#1469693 , @

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

lebedev.ri added a comment. The `APSInt.h` itself looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

lebedev.ri added a comment. Abandon this? Sorry for this fruitless effort. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60778/new/ https://reviews.llvm.org/D60778 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:12 +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + This looks wrong Comment at: clang-tools-extra/clang-tidy/linuxkernel/M

[PATCH] D60990: [Driver] Support priority for multilibs

lebedev.ri added a comment. Can this have test coverage? Comment at: clang/lib/Driver/Multilib.cpp:22 #include +#include #include Doesn't seem to be used? Comment at: clang/lib/Driver/Multilib.cpp:271 + // Sort multilibs by priority and

[PATCH] D59725: Additions to creduce script

lebedev.ri added a comment. I've stumbled into an issue with the script: It got a line: `clang: /build/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2582: bool {anonymous}::IndVarSimplify::run(llvm::Loop*): Assertion `L->isRecursivelyLCSSAForm(*DT, *LI) && "LCSSA required to run indvars!"' fai

[PATCH] D59725: Additions to creduce script

lebedev.ri added inline comments. Comment at: cfe/trunk/utils/creduce-clang-crash.py:185 +for msg in self.expected_output: + output += 'grep %s t.log || exit 1\n' % pipes.quote(msg) + >>! In D59725#1477362, @arichardson wrote: >>>! In D59725#1477042, @le

[PATCH] D59725: Additions to creduce script

lebedev.ri added inline comments. Comment at: cfe/trunk/utils/creduce-clang-crash.py:185 +for msg in self.expected_output: + output += 'grep %s t.log || exit 1\n' % pipes.quote(msg) + akhuang wrote: > lebedev.ri wrote: > > >>! In D59725#1477362, @arichar

[PATCH] D59725: Additions to creduce script

lebedev.ri added inline comments. Comment at: cfe/trunk/utils/creduce-clang-crash.py:185 +for msg in self.expected_output: + output += 'grep %s t.log || exit 1\n' % pipes.quote(msg) + akhuang wrote: > lebedev.ri wrote: > > akhuang wrote: > > > lebedev.ri

[PATCH] D61209: [clang-tidy] Fix readability-redundant-smartptr-get for MSVC STL

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp:60 // Catch '!ptr.get()' - const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl( - QuacksLikeASmartptr, has(cxxConversionDecl(returns(

[PATCH] D61288: [Diagnostics] Implemented support for -Wswitch-default

lebedev.ri added a comment. In D61288#1486008 , @aaron.ballman wrote: > In D61288#1486006 , @xbolva00 wrote: > > > Some coding guidelines may require switch to have always default label. > > Even if devs know that

[PATCH] D60760: Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO

lebedev.ri added a comment. Did this get reviewed? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60760/new/ https://reviews.llvm.org/D60760 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D60760: Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO

lebedev.ri added a comment. In D60760#1487387 , @sberg wrote: > In D60760#1487342 , @lebedev.ri > wrote: > > > Did this get reviewed? > > > I didn't get any responses at all, so decided to push it anyway. That is

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. The tests should be fixed to use `FileCheck`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61496/new/ https://reviews.llvm.org/D61496 ___

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Sure, this looks better. Comment at: clang/trunk/test/Preprocessor/indent_macro.c:1-2 -// RUN: %clang_cc1 -E %s | grep '^ zzap$' +// RUN: %clang_cc1 -E %s | FileChe

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

lebedev.ri added reviewers: aaron.ballman, lebedev.ri. lebedev.ri added a comment. I recommend to split this into two parts - changing `PragmaIntroducerKind Introducer` to `struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};`, and the actual openmp change. For that change, just basin

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

lebedev.ri added a comment. In D61509#1491158 , @jdenny wrote: > In D61509#1491119 , @lebedev.ri > wrote: > > > I recommend to split this into two parts - changing `PragmaIntroducerKind > > Introducer` to > > `st

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

lebedev.ri added a comment. In D61509#1491397 , @jdenny wrote: > In D61509#1491209 , @ABataev wrote: > > > In D61509#1491204 , @lebedev.ri > > wrote: > > > > > In D61509#149

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

lebedev.ri added a comment. It would have been better to submit this refactor as a new patch.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61509/new/ https://reviews.llvm.org/D61509 ___ cfe-commits m

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

lebedev.ri added a comment. In D61509#1493714 , @jdenny wrote: > In D61509#1493703 , @lebedev.ri > wrote: > > > It would have been better to submit this refactor as a new patch.. > > > Sorry, I didn't realize that

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

lebedev.ri added a comment. I don't see why this differential can't be updated to only contain the remaining part of the diff (for the actual OpenMP change), after splitting the NFC refactoring part. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6

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

lebedev.ri added reviewers: rsmith, aaron.ballman. lebedev.ri added a comment. Seems like obvious NFC to me, but i'm not sure about API/ABI implications for external use. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") {

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

lebedev.ri added inline comments. Comment at: lib/Driver/ToolChains/Linux.cpp:674 // removing them. - "/usr/include/i686-linux-gnu/64", "/usr/include/i486-linux-gnu/64"}; const StringRef X86MultiarchIncludeDirs[] = { This *might* be what clang-form

[PATCH] D52120: [analyzer] Treat std::{move, forward} as casts in ExprMutationAnalyzer.

lebedev.ri added a comment. In https://reviews.llvm.org/D52120#1235515, @lebedev.ri wrote: > Though some of that is still false-positives (pointers, va_arg, callback > lambdas passed as templated function param), but i'll file further bugs i > guess. Filed the ones that i can pinpoint, marked

[PATCH] D52135: [Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about variable cast to void

lebedev.ri added a comment. This looks weird. It will now completely skip all the explicitly instantiated functions, no? I'd think the problem that needs to be fixed is that it considers the local variable `int a_template;` to be in the function argument list. Repository: rCTE Clang Tools Ext

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

lebedev.ri added inline comments. Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:73-74 +void ConcatNestedNamespacesCheck::registerMatchers(MatchFinder *Finder) { + if (getLangOpts().CPlusPlus) +Finder->addMatcher(namespaceDecl().bind("namespace"), this); +}

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

lebedev.ri added inline comments. Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:42-45 + if (Decl->getKind() != Decl::Kind::Namespace) +return false; + + auto const *NS = reinterpret_cast(Decl); Use proper casting, `isa<>()`, `dyn_cast<>`,

[PATCH] D52137: Added warning for unary minus used with unsigned type

lebedev.ri added a comment. I don't think this makes much sense. In https://reviews.llvm.org/D52137#1236011, @xbolva00 wrote: > /home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: > unary minus operator applied to type 'unsigned long long', result value is > still unsi

[PATCH] D52137: Added warning for unary minus used with unsigned type

lebedev.ri added a comment. In https://reviews.llvm.org/D52137#1236011, @xbolva00 wrote: > /home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: > unary minus operator applied to type 'unsigned long long', result value is > still unsigned > > return __X & -__X; > > >

[PATCH] D52137: Added warning for unary minus used with unsigned type

lebedev.ri added a comment. > found some occurrences of this expression on our code base. Standard question: what is the SNR of this warning? What value it brings? How many times it fired? How many of these are actual real bugs? https://reviews.llvm.org/D52137 ___

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

lebedev.ri added a comment. In https://reviews.llvm.org/D50250#1236478, @rjmccall wrote: > It might help if you're more specific about whose review you're asking for. I suppose the main suspect is still the @rsmith. Though, https://reviews.llvm.org/D50901 is less controversial, so maybe best to

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

lebedev.ri added a comment. @rsmith - ping. This one should be rather uncontroversial i think? Is this moving in the direction you suggested? :) Repository: rC Clang https://reviews.llvm.org/D50901 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

lebedev.ri added a comment. @vsk thanks for taking a look! Comment at: lib/CodeGen/CGExprScalar.cpp:305 enum ImplicitConversionCheckKind : unsigned char { -ICCK_IntegerTruncation = 0, +ICCK_IntegerTruncation = 0, // Legacy, no longer used. +ICCK_UnsignedIntegerTr

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

lebedev.ri added a comment. In https://reviews.llvm.org/D50901#1238795, @lebedev.ri wrote: > @vsk thanks for taking a look! /me can't read :) That was supposed to be @vitalybuka, of course (: Repository: rC Clang https://reviews.llvm.org/D50901 __

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

lebedev.ri added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:27-32 + // template + // void f(T func) { + // func(); + // ^~~ + // ::std::invoke(func, 1) + Finder->addMatcher(callExpr(has(declRefExpr())).bind("functor"), thi

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a comment. This looks questionable to me. I don't disagree with the reasons stated about llvm types. But is that *always* true? I would personally be very surprized, and consider this a false-positive. This should at least be optional. Not sure about the default, but setting the

[PATCH] D52313: [clang-doc] Avoid parsing undefined base classes

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. If it **no longer** crashes, i guess you can test for that? Other than that, SG. https://reviews.llvm.org/D52313 ___ cfe-commits mailing

[PATCH] D52248: [SEMA] ignore duplicate declaration specifiers from typeof exprs

lebedev.ri added inline comments. Comment at: test/Sema/gnu89.c:1-2 -// RUN: %clang_cc1 %s -std=gnu89 -pedantic -fsyntax-only -verify +// RUN: %clang_cc1 %s -std=gnu89 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-PEDANTIC %s +// RUN: %clang_cc1 %s -std=gnu89 -fsy

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

lebedev.ri added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:27-32 + // template + // void f(T func) { + // func(); + // ^~~ + // ::std::invoke(func, 1) + Finder->addMatcher(callExpr(has(declRefExpr())).bind("functor"), thi

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a comment. In https://reviews.llvm.org/D52315#1241487, @baloghadamsoftware wrote: > I still wonder what true positives could we get without checking the size? No > real-life types come to my mind with size of a pointer but really expensive > copy operations. What could be so re

[PATCH] D45179: [libc++] Add _LIBCPP_ENABLE_NODISCARD and _LIBCPP_NODISCARD_EXT to allow pre-C++2a [[nodiscard]]

lebedev.ri added a comment. In https://reviews.llvm.org/D45179#1225911, @EricWF wrote: > I don't think it ever landed. I'll try to get it in this week. What does this need? Is there some changes missing still? Or this simply needs to be committed (and bots watched?) https://reviews.llvm.org/D

[PATCH] D52300: [clangd] Implement VByte PostingList compression

lebedev.ri added a comment. In https://reviews.llvm.org/D52300#1241776, @ioeric wrote: > In https://reviews.llvm.org/D52300#1241754, @kbobyrev wrote: > > > Also, I'll refine https://reviews.llvm.org/D52047 a little bit and I > > believe that is should be way easier to understand performance + me

[PATCH] D52047: [clangd] Add building benchmark and memory consumption tracking

lebedev.ri added inline comments. Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:81 +// Same for the next "benchmark". +// FIXME(kbobyrev): Should this be separated into the BackingMemorySize +// (underlying SymbolSlab size) and Symbol Index (MemIndex/Dex) ove

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a reviewer: lebedev.ri. lebedev.ri added a comment. Better, but these blacklists need to be config options, not random hardcoded globs. See e.g. `google-runtime-references.WhiteListTypes`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52360

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

lebedev.ri added a comment. `#ifdef` hell is usually messy and is a source of problems. May i ask what is the motivation for this change? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52334 ___ cfe-commits mailing list cfe-commits@

[PATCH] D52390: [analyzer] StackSizeChecker

lebedev.ri added a comment. Without seeing the full picture (i.e. what you want to do with the clang-tidy check) it is hard to tell, but are you *very* sure all this logic should be in `clang/lib/StaticAnalyzer/`, and not in `clang/lib/Analysis/` ? Repository: rC Clang https://reviews.llvm.

[PATCH] D45179: [libc++] Add _LIBCPP_ENABLE_NODISCARD and _LIBCPP_NODISCARD_EXT to allow pre-C++2a [[nodiscard]]

lebedev.ri added a comment. In https://reviews.llvm.org/D45179#1242896, @EricWF wrote: > In https://reviews.llvm.org/D45179#1241721, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D45179#1225911, @EricWF wrote: > > > > > I don't think it ever landed. I'll try to get it in this week. > > > >

[PATCH] D45179: [libc++] Add _LIBCPP_ENABLE_NODISCARD and _LIBCPP_NODISCARD_EXT to allow pre-C++2a [[nodiscard]]

lebedev.ri commandeered this revision. lebedev.ri removed a reviewer: lebedev.ri. lebedev.ri added a comment. In https://reviews.llvm.org/D45179#1242915, @EricWF wrote: > @lebedev.ri Sure, be my guest. It frees me up to do reviews. Thank you. Np. > I just finished running the test suite, so it

[PATCH] D52390: [analyzer] StackSizeChecker

lebedev.ri added a comment. In https://reviews.llvm.org/D52390#1243228, @mate1214 wrote: > Hi @lebedev.ri! > > Thanks for the question. I was not sure as to where exactly put the files. > The important thing for me is that the `StackUsageMeasuringVisitor` should be > reachable from the clang-ti

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

lebedev.ri added a comment. Thanks! Comment at: lib/Sema/SemaDecl.cpp:12380-12382 } + if (LangOpts.CPlusPlus && II) { I think you could move it into the `if()` above? https://reviews.llvm.org/D52421 ___ cfe

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

lebedev.ri created this revision. lebedev.ri added reviewers: sylvestre.ledru, alexfh, jroelofs, ygribov. Herald added subscribers: Szelethus, mikhail.ramalho, a.sidorin, szepet, xazax.hun. Herald added a reviewer: george.karpenkov. This has been bothering me for a while, but only now i have actu

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

lebedev.ri updated this revision to Diff 167060. lebedev.ri added a reviewer: krememek. lebedev.ri added a comment. Updated the `--help`, too. Repository: rC Clang https://reviews.llvm.org/D52530 Files: tools/scan-build/bin/scan-build Index: tools/scan-build/bin/scan-build ==

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

lebedev.ri added a comment. In https://reviews.llvm.org/D52334#1246356, @steveire wrote: > Can I go ahead and merge this now? In https://reviews.llvm.org/D52334#1242813, @lebedev.ri wrote: > `#ifdef` hell is usually messy and is a source of problems. > May i ask what is the motivation for t

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a comment. In https://reviews.llvm.org/D52360#1246440, @baloghadamsoftware wrote: > Config option is a good idea but it must not be empty by default. The checker > must ignore all pointer and references by default based on their names. I disagree, it **must** not have false-ne

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

lebedev.ri added a comment. In https://reviews.llvm.org/D52530#1246459, @jroelofs wrote: > LGTM Thank you for the speedy review! Repository: rC Clang https://reviews.llvm.org/D52530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. This looks good to me, but you probably want to wait for someone else to review this, too. https://reviews.llvm.org/D52421 ___ cfe-commi

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a comment. In https://reviews.llvm.org/D52360#1246474, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D52360#1246452, @lebedev.ri wrote: > > > I disagree, it **must** not have false-negatives by default. > > > What kind of false-negatives could be caused by smart pointe

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

lebedev.ri added a comment. In https://reviews.llvm.org/D52315#1245747, @dblaikie wrote: > ... I think this has been attempted to be superseded by https://reviews.llvm.org/D52360, although that has the same basic issue. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52315

[PATCH] D52552: [clang-tidy] Flag Classes Inheriting From Structs

lebedev.ri added a comment. Please upload patches with full context. Please do follow coding style, clang-format your patches. Would it make sense to not catch-all all the `class : struct`, but consider the explicitly specified inheritance visibility? Comment at: clang-tidy/m

[PATCH] D52552: [clang-tidy] Flag Classes Inheriting From Structs

lebedev.ri added inline comments. Comment at: test/clang-tidy/misc-class-inherit-from-struct.cpp:13 +}; + +class C lebedev.ri wrote: > Missing cases: > * struct inheriting from struct > * Different inheritance visibility: > * You only check the default visibili

[PATCH] D52589: [clang][ubsan][NFC] Slight test cleanup in preparation for D50901

lebedev.ri created this revision. lebedev.ri added reviewers: vsk, vitalybuka, filcab. Repository: rC Clang https://reviews.llvm.org/D52589 Files: test/CodeGen/catch-implicit-integer-conversions-basics.c test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c test/CodeGen/catc

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

lebedev.ri updated this revision to Diff 167255. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. - Split off the NFC preparatory changes. - Rebased. Repository: rC Clang https://reviews.llvm.org/D50901 Files: docs/UndefinedBehaviorSanitizer.rst include/clang/Basi

[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

lebedev.ri added inline comments. Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343 + auto Diag = + diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables") + << static_cast( kbobyrev wrote: > JonasToth wrote: > > kbobyrev wrote

[PATCH] D52589: [clang][ubsan][NFC] Slight test cleanup in preparation for D50901

lebedev.ri added a comment. Thank you for the review! Comment at: test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c:7 +//// +// Unsigned case. +//---

[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

lebedev.ri updated this revision to Diff 167391. lebedev.ri marked an inline comment as done and 3 inline comments as not done. lebedev.ri added a comment. - Rebased Repository: rC Clang https://reviews.llvm.org/D50901 Files: docs/UndefinedBehaviorSanitizer.rst include/clang/Basic/Saniti

<    3   4   5   6   7   8   9   10   11   12   >