[PATCH] D128359: [clang][dataflow] Move logic for `createStorageLocation` from `DataflowEnvironment` to `DataflowAnalysisContext`.

gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:47 +/// Gets the set of all fields in the type. +llvm::DenseSet getObjectFields(QualType Type); Repository:

[PATCH] D126316: [clang][dataflow] Make limit on fixpoint-algorithm iterations proportional to size of CFG.

gribozavr2 added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:336 + static constexpr uint32_t MaxAverageVisitsPerBlock = 4; + const uint32_t MaxIterations = MaxAverageVisitsPerBlock * BlockStates.size(); uint32_t Iterations = 0

[PATCH] D126973: [clang][dataflow] Relax assumption that `AggregateStorageLocations` correspond to struct type.

gribozavr2 added a comment. > using the StructValue for its properties, while modeling a non-struct type. Could you explain the use case in more details? Maybe a better change is to move synthetic properties to the base class `Value`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D127114: new clang-tidy checker for assignments within condition clause of if statement

gribozavr2 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:9 +Finds these assignments even within multiple sets of parentheses which is often appropriate to structure multi-part condition statements. +Finds these assi

[PATCH] D127114: new clang-tidy checker for assignments within condition clause of if statement

gribozavr2 added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/AssignmentInIfClauseCheck.cpp:39 + diag(MatchedDecl->getBeginLoc(), + "Assignment detected within if statement. Fix to equality check if this " + "was accidental. Consider moving out of

[PATCH] D127196: [clang][dataflow] Enable use of synthetic properties on all Value instances.

gribozavr2 added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:29-30 /// Base class for all values computed by abstract interpretation. +/// All Value instances should be separately allocated and stored by pointer +/// for pointer stability. cl

[PATCH] D127312: [clang][dataflow] Remove IndirectionValue class, moving PointeeLoc field into PointerValue and ReferenceValue

gribozavr2 added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:53 +static bool indirectionEquivalentValues(Value *Val1, Value *Val2) { + if (auto *IndVal1 = dyn_cast(Val1)) { WDYT about "areEquivalentIndirectionValues"?

[PATCH] D127196: [clang][dataflow] Enable use of synthetic properties on all Value instances.

gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:28 /// Base class for all values computed by abstract interpretation. +/// Don't use `Value` instances by value. All `Value` instances are alloca

[PATCH] D127196: [clang][dataflow] Enable use of synthetic properties on all Value instances.

This revision was automatically updated to reflect the committed changes. Closed by commit rG49ed5bf51958: [clang][dataflow] Enable use of synthetic properties on all Value instances. (authored by wyt, committed by gribozavr). Changed prior to commit: https://reviews.llvm.org/D127196?vs=435121&

[PATCH] D127312: [clang][dataflow] Remove IndirectionValue class, moving PointeeLoc field into PointerValue and ReferenceValue

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:63 -auto *IndVal2 = cast(Val2); -assert(IndVal1->getKind() == IndVal2->getKind()); -

[PATCH] D127312: [clang][dataflow] Remove IndirectionValue class, moving PointeeLoc field into PointerValue and ReferenceValue

This revision was automatically updated to reflect the committed changes. Closed by commit rGa1b2b7d9790b: [clang][dataflow] Remove IndirectionValue class, moving PointeeLoc field into… (authored by wyt, committed by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D155506: [clang][JumpDiagnostics] use StmtClass rather than dyn_cast chain NFC

gribozavr2 added a comment. I'm not sure it is actually an anti-pattern. `dyn_cast` will transparently handle subclasses, should any be added in the future, but a switch wouldn't. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155506/new/ https://r

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Thank you! Do you have commit access or do you need me to commit this patch for you? Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessMode

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

gribozavr2 added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10325 - for (const auto *FD : RD.fields()) { -// Ill-formed if the field is an ObjectiveC pointer or of a type that is -// non-trivial for the purpose of calls. -QualType FT = FD->getType(

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

gribozavr2 added a comment. In D155890#4523243 , @adukeman wrote: > In D155890#4522266 , @ymandel wrote: > >> In D155890#4521266 , >> @carlosgalvezp wrote: >> >>> This sh

[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

gribozavr2 added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62 +return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") || +isTopLevelNamespaceWithName(*N, "folly")); }

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

gribozavr2 added a comment. @lukasza I think you forgot to upload an updated version of the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155895/new/ https://reviews.llvm.org/D155895 ___ cfe-commi

[PATCH] D156728: [Sema] Ignore nullability qualifiers when deducing types for `auto` and `__auto_type`

gribozavr2 added a comment. I understand that this is an expedient fix that silences the warning. However, the fundamental problem is the simplistic implementation of the warning (that it is not flow-sensitive), not the inference of nullability. That is, when we are removing nullability qualifi

[PATCH] D156728: [Sema] Ignore nullability qualifiers when deducing types for `auto` and `__auto_type`

gribozavr2 added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:3933 // type are ignored for type deduction. + // Ignore top level nullability qualifiers too. ArgType = ArgType.getUnqualifiedType(); ahatanak wrote: > gr

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:258 + static_assert(__is_trivially_relocatable(TrivialAbiAttributeAppliedToAnonymousUnion), ""); +#endif +

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbddaa3517786: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`. (authored by gribozavr). Changed prior to commit: https://revie

[PATCH] D153805: Expose DataflowAnalysisContext.querySolver().

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3b29b8a2aba2: Expose DataflowAnalysisContext.querySolver(). (authored by bazuzi, committed by gribozavr). Repository: rG LLVM Github Monorepo CHA

[PATCH] D153006: [clang][dataflow] Perform deep copies in copy and move operations.

gribozavr2 added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/RecordOps.h:26 +/// fields of record type. It also copies properties from the `StructValue` +/// associated with `Dst` to the `StructValue` associated with `Src` (if these +/// `StructValue`s

[PATCH] D154339: [clang][dataflow] Make `runDataflowReturnError()` a non-template function.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:389 /// verify the results. -template -llvm::Error -runDataflowReturnError(llvm::StringRef C

[PATCH] D154339: [clang][dataflow] Make `runDataflowReturnError()` a non-template function.

gribozavr2 added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:389 /// verify the results. -template -llvm::Error -runDataflowReturnError(llvm::StringRef Code, VerifyResultsT VerifyResults, - DataflowAnalysisOptions O

[PATCH] D154339: [clang][dataflow] Make `runDataflowReturnError()` a non-template function.

gribozavr2 added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:389 /// verify the results. -template -llvm::Error -runDataflowReturnError(llvm::StringRef Code, VerifyResultsT VerifyResults, - DataflowAnalysisOptions O

[PATCH] D154935: [clang][dataflow] Introduce `getFieldValue()` test helpers.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp:27 using namespace dataflow; +using test::getFieldValue; using ::testing::Elemen

[PATCH] D154948: [dataflow] improve determinism of generated SAT system

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Analysis/FlowSensitive/DeterminismTest.cpp:62 +}; +struct Tree{ + int height(); Repository: rG LLVM

[PATCH] D155202: [clang][dataflow] Simplify implementation of `transferStdForwardCall()` in optional check.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:683 - StorageLocation *LocRet = State.Env.getStorageLocation(*E, SkipPast

[PATCH] D76497: [Syntax] Test both the default and windows target platforms in unittests

This revision was automatically updated to reflect the committed changes. Closed by commit rGeddede9d5184: [Syntax] Test both the default and windows target platforms in unittests (authored by hlopko, committed by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D76355: [Syntax] Build mapping from AST to syntax tree nodes

gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:173 +private: + // Keys are either Stmt* or Decl*. + llvm::DenseMap Nodes; The comment is not needed anymore. Comment at: clang/lib/Tooling/Syntax/BuildTree.

[PATCH] D72446: [Syntax] Build mapping from AST to syntax tree nodes

gribozavr2 closed this revision. gribozavr2 added a comment. Superseded by https://reviews.llvm.org/D76355. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72446/new/ https://reviews.llvm.org/D72446 ___

[PATCH] D76355: [Syntax] Build mapping from AST to syntax tree nodes

gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:129 + void SetRole(NodeRole NR); + `setRole()` (in new code). Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:975 + const syntax::Token *Template

[PATCH] D76355: [Syntax] Build mapping from AST to syntax tree nodes

This revision was automatically updated to reflect the committed changes. Closed by commit rGa711a3a46039: [Syntax] Build mapping from AST to syntax tree nodes (authored by hlopko, committed by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D76761: [clang-tidy] Fix crash in readability-redundant-string-cstr

gribozavr2 added a comment. Could you provide more information about why these null checks are needed in this case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76761/new/ https://reviews.llvm.org/D76761 ___

[PATCH] D76761: [clang-tidy] Fix crash in readability-redundant-string-cstr

gribozavr2 added a comment. In D76761#1941380 , @njames93 wrote: > In D76761#1941070 , @gribozavr2 > wrote: > > > Could you provide more information about why these null checks are needed > > in this case? > > > W

[PATCH] D76922: [Syntax] Remove delayed folding from tree building.

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Could you add the following tests? struct Point { int x, y; } point, *pointPtr; typedef struct Point { int x, y; } PointTy, *PointPtr; typedef struct { int x, y; } PointTy, *Point

[PATCH] D76922: [Syntax] Remove delayed folding from tree building.

This revision was automatically updated to reflect the committed changes. Closed by commit rGcdce2fe561eb: [Syntax] Remove delayed folding from tree building. (authored by hlopko, committed by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D75040: [ASTMatchers] Adds a matcher called `hasAnyOperatorName`

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1870 +std::is_same::value, +"unsupported class for matcher")

[PATCH] D75040: [ASTMatchers] Adds a matcher called `hasAnyOperatorName`

gribozavr2 added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4772 +/// +/// FIXME: Tweak to improve docs generated +extern const internal::VariadicFunctionhttps://reviews.llvm.org/D75040/new/ https://reviews.llvm.org/D75040 _

[PATCH] D72153: [libTooling] Add function to determine associated text of a declaration.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Transformer/SourceCode.h:42 +/// terminators. The returned range consists of file locations, if valid file +/// locations

[PATCH] D75289: [clang-tidy] Added virtual isLanguageVersionSupported to ClangTidyCheck

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:86 /// matches occur in the order of the AST traversal. virtual void registerMatchers(ast_matchers

[PATCH] D75289: [clang-tidy] Added virtual isLanguageVersionSupported to ClangTidyCheck

gribozavr2 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:109 + /// \p LangOpts. + virtual bool isLanguageVersionSupported(const LangOptions &LangOpts) const { +return true; Not a critical thing, but I feel like it would be

[PATCH] D75340: [clang-tidy] Change checks to use new isLanguageVersionSupported restriction

gribozavr2 added a comment. Looks good, but please move non-mechanical changes (potentially-semantic changes) into a separate patch. Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.h:30 + bool isLanguageVersionSupported(const LangOptions &LangO

[PATCH] D75365: [AST Matchers] Fix bug in 'optionally' matcher wherein all previous bindings are cleared when all inner matchers fail.

gribozavr2 added a comment. +1 to this fix. However, regarding `allOf` vs. `anyOf` semantics, since `optionally` always succeeds, is there a difference between the two semantics? It seems to me that there should be no difference between `allOf(optionally(a), optionally(b))` vs. `anyOf(optional

[PATCH] D75365: [AST Matchers] Fix bug in 'optionally' matcher wherein all previous bindings are cleared when all inner matchers fail.

gribozavr2 added a comment. > I think the difference is in whether you continue with the submatchers after > a success. Allof does while anyof does not. Oh, the short-circuiting makes a difference! I see. +1 to making it non-variadic then. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D75441: [clang-tidy] Add helper base check classes that only run on specific language versions

gribozavr2 added a comment. > I think my preference is to go with isLanguageVersionSupported() and not use > base classes. +1, I can see this working, but the introduction of new concepts does not seem like it pulls its weight given the tiny amount of boilerplate that they eliminate. Reposit

[PATCH] D75340: [clang-tidy] Change checks to use new isLanguageVersionSupported restriction

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.h:30 + bool isLanguageVersionSupported(const LangOptions &LangOpts) const overrid

[PATCH] D75538: [clang-tidy] Updated language supported restrictions on some checks

gribozavr2 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-basic.cpp:1 -// RUN: %check_clang_tidy -std=c++98 %s modernize-use-nullptr %t -- -- -Wno-non-literal-null-conversion -// njames93 wrote: > alexfh wrot

[PATCH] D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix

gribozavr2 requested changes to this revision. gribozavr2 added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:21 +const char DefaultStringNames[] = "basic_string"; + ---

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

gribozavr2 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:67-70 +// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to +// return CtorExpr->getSourceRange(). However, starting with c++17, parsing +//

[PATCH] D69613: [libTooling] Simplify type structure of `Stencil`s.

gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:57 + /// Constructs a string representation of the StencilInterfacePart. + /// StencilInterfaceParts generated by the `selection` and `run` functions do + /// not have a unique stri

[PATCH] D69435: [clang-tidy] New checker performance-trivially-destructible-check

gribozavr2 added a comment. LGTM modulo the isFirstDeclComment. Will approve after we resolve that discussion. Comment at: clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp:45 + isDefaulted(), + unless(anyOf(isFirstDecl(), isVirtual(),

[PATCH] D69435: [clang-tidy] New checker performance-trivially-destructible-check

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Alright then. (Although I don't know whether the redeclaration chain always models that wording in the standard, and what that wording means when we don't have a linear ordering of tex

[PATCH] D69613: [libTooling] Simplify type structure of `Stencil`s.

gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:82 +Stencil makeStencil(RangeSelector Selector); +inline Stencil makeStencil(Stencil S) { return S; } ymandel wrote: > gribozavr2 wrote: > > I feel like this should

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

gribozavr2 added a comment. I fully agree about passing a Stencil to `access`. However whether to call `makeStencil` inside is an interesting question. On one hand, such implicit conversions increase the convenience. On the other, they increase the API surface (more possible argument types), an

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

gribozavr2 added a comment. > Which idiom do you think we should encourage, then, for text and > range-selectors -- the named combinator or the single-argument cat? That is You are making a very interesting point! It sounds very appealing to me to remove the special `text` and `selection` func

[PATCH] D69613: [libTooling] Simplify type structure of `Stencil`s.

gribozavr2 added a comment. I see. Some decoupling is desirable, I agree. Maybe move `StencilInterface` and `Stencil` into a separate header that `RewriteRule` can depend on, and then define the library of stencils in another header? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

gribozavr2 added a comment. > Are you suggesting we remove text and selection entirely? Yes! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69625/new/ https://reviews.llvm.org/D69625 ___ cfe-commits ma

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp:180 +// use((const S*)this); +// (const S*)->f() +// when 'f'

[PATCH] D69184: [libTooling] Introduce general combinator for fixed-value `MatchConsumer`s.

gribozavr2 added a comment. > In fact, it might be a good idea to rename `change` to `changeTo` (WDYT?). I like it. It should combine well with all stencils, not just `text`. > An alternative, is not to introduce a new combinator at all, and just use > Stencils: I think we are converging on th

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

gribozavr2 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp:321 + +} // namespace Keep Could you add tests for calls to members of `this` through member function pointers? I don't care about possibl

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 accepted this revision. gribozavr2 added a comment. LGTM for ClangTidy, assuming you ran `ninja check-all`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69950/new/ https://reviews.llvm.org/D69950 ___ cfe-commits mailing list cfe

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. > Should I go ahead and revert my patch while I investigate the issue on > Windows or should I just push a new patch deleting the 'CHECK-FIXES' while I > look into the issue? Yes. As a general rule, if you landed a patch and it broke a bot that was previously green,

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

gribozavr2 added a comment. If it is indeed the extra AST node for the elidable constructor, see if you can use the `ignoringElidableConstructorCall` AST matcher to ignore it, therefore smoothing over AST differences between language modes. Repository: rCTE Clang Tools Extra CHANGES SINCE L

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. A number of ClangTidy tests have `-fno-delayed-template-parsing` in their RUN lines. While not a great solution (it only fixes the test, but does not make the warnings show for users on Windows), there's only so much a checker can do to counteract that. Before adding

[PATCH] D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line

gribozavr2 added inline comments. Comment at: clang/include/clang/AST/CommentLexer.h:25 + +/// Requires that BufferPtr point to a newline character (/n or /r). +/// Returns a pointer past the end of any platform newline, i.e. past "\n" and "\r" (everywhere) ===

[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Thanks! Do you have commit access or do you need me to commit for you? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69238/new/ https://r

[PATCH] D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-redundant-string-init.rst:29 + +Semicolon-delimited list of base class names to apply this c

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. > hence not throwing the warning on any platform? I'm not sure I understand? The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced. Usually

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. In D69950#1751894 , @eandrews wrote: > In D69950#1751859 , @gribozavr2 > wrote: > > > > hence not throwing the warning on any platform? > > > > The way I read the buildbot breakage, an e

[PATCH] D70554: [libTooling] Add stencil combinators for nodes that may be pointers or values.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:95 +// Constructs an expression that idiomatically represents a value, taking into +// account whe

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

gribozavr2 added a comment. > This patch introduces a way to apply the fix-its by the Analyzer: I'm not sure this option is very useful... I don't know of anyone who uses the same option in Clang or ClangTidy. The only way I know people apply fixits is with the help of IDEs. I am also skeptical

[PATCH] D69223: WDocumentation: Implement the \anchor.

gribozavr2 added a comment. Thank you for the contribution and sorry for the review delay! Comment at: clang/bindings/xml/comment-xml-schema.rng:583 + + + Since the name of the anchor is not a part of the document text, it should be an at

[PATCH] D70554: [libTooling] Add stencil combinators for nodes that may be pointers or values.

gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:90 +/// If \p ExprId is of pointer type, constructs an idiomatic dereferencing of +/// the expression bound to \p ExprId, including wrapping it in

[PATCH] D70554: [libTooling] Add stencil combinators for nodes that may be pointers or values.

gribozavr2 marked an inline comment as done. gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:96 +// Constructs an expression that idiomatically represents a value, taking into +// account whether `ExprId` is a pointer or already a v

[PATCH] D70787: [Syntax] Build SimpleDeclaration node that groups multiple declarators

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:453 +)txt"}, + // Multiple declarators group into a single SimpleDeclaration. + {R"cpp(

[PATCH] D69223: WDocumentation: Implement the \anchor.

gribozavr2 added a comment. A few more comments, but generally looks good! Comment at: clang/include/clang-c/Documentation.h:188 + /** + * Command argument should not be rendered (since it is a only defines + * an anchor). "since it is a only" => "since it

[PATCH] D70856: [Syntax] Build nodes for simple cases of top level declarations

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:346 +/// static_assert(, ) +/// static_assert() +class StaticAssertDeclaration final : public Declaration {

[PATCH] D69223: WDocumentation: Implement the \anchor.

gribozavr2 marked an inline comment as done. gribozavr2 added inline comments. Comment at: clang/test/Index/comment-xml-schema.c:38 +// RUN: xmllint --noout --relaxng %S/../../bindings/xml/comment-xml-schema.rng %S/Inputs/CommentXML/valid-inline-command-01.xml // RUN: not xml

[PATCH] D70926: [clang-format] Add option for not breaking line before ObjC params

gribozavr2 added inline comments. Comment at: clang/include/clang/Format/Format.h:1651 + /// parameters in a fuction call. + bool ObjCDontBreakBeforeNestedBlockParam; + I don't know much about ClangFormat, but I'd prefer to keep the option name positive ("ObjC

[PATCH] D69223: WDocumentation: Implement the \anchor.

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. With that last comment, LGTM. Do you have commit access? Comment at: clang/lib/AST/TextNodeDumper.cpp:493 + case comments::InlineCommandComment::RenderAnchor: +O

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

gribozavr2 added a comment. @twardakm: I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D70974: [clang-tidy] Fix PR26274

gribozavr2 added a reviewer: gribozavr2. gribozavr2 added a comment. I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias? Repository: rG LLVM Githu

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

gribozavr2 added a comment. Is this a common problem? There's a lot of silly code we could try to find, but if people don't actually write it, then we get all downsides of maintenance without the benefits of the checker. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https:/

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

gribozavr2 added a comment. In D71001#1768963 , @baloghadamsoftware wrote: > In D71001#1768880 , @gribozavr2 > wrote: > > > Is this a common problem? There's a lot of silly code we could try to find, > > but if p

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

gribozavr2 added a comment. In D71001#1769035 , @Eugene.Zelenko wrote: > > ASan can help debug this issue, and more. > > This is dynamic analysis, and detection of problem depends on test case. > Detection of such problem during static analysis makes sen

[PATCH] D70974: [clang-tidy] Fix PR26274

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. In D70974#1768902 , @aaron.ballman wrote: > In D70974#1768871 , @gribozavr2 > wrote: > > > I'm not co

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

gribozavr2 added a comment. In D71001#1769071 , @baloghadamsoftware wrote: > In D71001#1769018 , @gribozavr2 > wrote: > > > ASan can help debug this issue, and more. > > > ASan is too heavyweight for this simple p

[PATCH] D70974: [clang-tidy] Fix PR26274

gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:54 + const LangOptions &LangOpts) { + // Loc should be at the begin of the namespace decl (usually, `name

[PATCH] D69855: [clang-tidy] Fix llvm-namespace-comment for macro expansions

gribozavr2 added a comment. In D69855#1769318 , @twardakm wrote: > @alexfh, @gribozavr2, @aaron.ballman I think the best way out here is just to > implement the basic fix for the above problem and only allow to use macro > definition in closing comment a

[PATCH] D71001: [Clang-Tidy] New check: bugprone-misplaced-pointer-arithmetic-in-alloc

gribozavr2 added a comment. In D71001#1769394 , @Eugene.Zelenko wrote: > With such logic, Clang-tidy is maintenance burden: 368 unaddressed request in > Bugzilla is very telling. Doesn't that just prove the point that we already have a problem with too

[PATCH] D71140: [Wdocumentation] Properly place deprecated attribute

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/Sema/warn-documentation-fixits.cpp:83 + +#if __cplusplus >= 201402L + // expected-warning@+2 {{declaration is marked with '\deprecated' co

[PATCH] D71141: [Wdocumentation] Use C2x/C++14 deprecated attribute

gribozavr2 accepted this revision. gribozavr2 added a comment. LGTM with comments fixed. Comment at: clang/lib/AST/CommentSema.cpp:698 + // - In C2x/C++14 we prefer [[deprecated]] + // - If not found or an older C/C++ look for __attribute__((deprecated)) + String

[PATCH] D71140: [Wdocumentation] Properly place deprecated attribute

gribozavr2 marked an inline comment as done. gribozavr2 added inline comments. Comment at: clang/test/Sema/warn-documentation-fixits.cpp:4 +// RUN: %clang_cc1 -std=c++14 -fsyntax-only -Wdocumentation -Wdocumentation-pedantic -fcomment-block-commands=foobar -fdiagnostics-parseab

[PATCH] D71141: [Wdocumentation] Use C2x/C++14 deprecated attribute

gribozavr2 marked an inline comment as done. gribozavr2 added inline comments. Comment at: clang/test/Sema/warn-documentation-fixits.c:27 + +// CHECK: fix-it:"{{.*}}":{7:1-7:1}:"[[ATTRIBUTE]] " +// CHECK: fix-it:"{{.*}}":{11:1-11:1}:"[[ATTRIBUTE]] " Mordante wrot

[PATCH] D71466: [ARM][MVE][Intrinsics] remove extraneous intrinsics.

gribozavr2 added a comment. I reverted this change in 34536db7bbe0b8c5f8ffa70df307312b451aca2e . This change didn't compile: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/20462. Please always run `ninja ch

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Generally looks good, just nitpicks. Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:24 const clang::TranslationU

[PATCH] D70872: [clangd] Implement "textDocument/documentLink" protocol support

gribozavr2 added a comment. Sorry, I reverted this change in 079ef783dd5530b5f87beefe624b9179547ded7e . The tests depend on builtin headers, which is not intentionally supported in clangd tests; these tests are broken in some

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

gribozavr2 added a comment. Updated version LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64573/new/ https://reviews.llvm.org/D64573 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

<    5   6   7   8   9   10   11   12   >