[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

davrec added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3630-3632 do { - ++CurPtr; -} while (isHorizontalWhitespace(*CurPtr)); + ++CurOffset; +} while (isHorizontalWhitespace(BufferStart[CurOffset])); davrec wrote: > ``` > for

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

davrec added a comment. Suggested a few adjustments in `LexTokenInternal` you might want to test for perf improvements (orthogonal to this patch, but could boost its numbers :). And also noted that `Lexer::getBuffer()` has same issue as `getBufferLocation()` re potential for pointer

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

davrec added a comment. Only had a chance to give it a once over, I will look through more closely later, def by this weekend. Main thing is I think we shouldn't be exposing the buffer pointers after this change, i.e. no public function should return `const char *`, unless I'm missing

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + mizvekov wrote: > mizvekov wrote: > > davrec wrote: > > > davrec wrote: > > > > I don't object with

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList *getReplacedTemplateParameterList(Decl *D); + davrec wrote: > I don't object with making this public, and I can see the argument for making > this its

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added a comment. I like the late changes, just need to add comments to the public methods, and maybe move `getReplacedTemplateParameterList` over to `Decl`. Comment at: clang/include/clang/AST/DeclTemplate.h:3427 +TemplateParameterList

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec accepted this revision. davrec added a comment. Looks good, over to @ChuanqiXu Comment at: clang/include/clang/Sema/Template.h:80 +struct ArgumentListLevel { + Decl *AssociatedDecl; + ArgList Args; mizvekov wrote: > davrec wrote: > >

[PATCH] D127695: [clang] Implement Template Specialization Resugaring

davrec added a subscriber: rjmccall. davrec added a comment. First thank you for having separated out the public AST changes into other patches, it makes these mostly-Sema changes much easier to review. I don't see any major issues with the code, though this would benefit from a close look

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added a comment. In D131858#3817646 , @mizvekov wrote: > In D131858#3815057 , @davrec wrote: > >>> So this patch at least puts that into consistency. >> >> Is there already an analogous breakage for

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

davrec added a comment. > I will give an example using a Typedef for exposition, but the same > motivation applies with UsingType. > > Say we have this code: > > template struct A { using type1 = T; }; > using Int = int; > > using type2 = A::type1; > > See this example live (with

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added a subscriber: sammccall. davrec added a comment. My concerns have been addressed, except for some more nitpicking on names which I think are uncontroversial. Regarding the modules breakage found by @ChuanqiXu: > Well we touch FunctionTemplates and VariableTemplates in this patch,

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

davrec added a subscriber: sammccall. davrec added a comment. > In general, forgetting about just the above particular way of creating them, > there might still exist only one TypedefDecl in the whole program, which > every TypedefType is referencing, and they can still be divergent, ie have >

[PATCH] D132816: [clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type

davrec accepted this revision. davrec added a comment. Agree this needs a brief commit message, just explaining that previously the underlying type had to be canonical, but now it can be sugared, allowing a better/more informative representation. Comment at:

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

davrec added a comment. If I understand this correctly: whenever isDivergent() is true, `getDecl()` or `getFoundDecl()` is an arbitrary choice between multiple candidate (re)declarations of a typedef or using decl. In particular it will be the first redeclaration, //even though each different

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12367 + case Type::Class: \ +llvm_unreachable("Unexpected " Kind ": " #Class); + aaronpuchert wrote: > mizvekov wrote: > >

[PATCH] D111509: [clang] use getCommonSugar in an assortment of places

davrec added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1113-1114 +// "double _Complex" is promoted to "long double _Complex". +static QualType handleComplexFloatConversion(Sema , ExprResult , + QualType LHSType, QualType

[PATCH] D111509: [clang] use getCommonSugar in an assortment of places

davrec added a comment. The lines you changed (clang/lib/Sema/SemaExpr.cpp:1091-1150) look good (IIUC you just change cast to cast_as and dyn_cast to getAs, and reorganize for clarity). I suggested some renaming and a few more comments to further clarify. Comment at:

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12367 + case Type::Class: \ +llvm_unreachable("Unexpected " Kind ": " #Class); + mizvekov wrote: > davrec wrote: > > mizvekov

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12367 + case Type::Class: \ +llvm_unreachable("Unexpected " Kind ": " #Class); + mizvekov wrote: > davrec wrote: > > Could we

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12367 + case Type::Class: \ +llvm_unreachable("Unexpected " Kind ": " #Class); + Could we just return `X` here? Would that

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

davrec added inline comments. Comment at: clang/include/clang/AST/Type.h:5530-5537 /// Represents a type that was referred to using an elaborated type /// keyword, e.g., struct S, or via a qualified name, e.g., N::M::type, /// or both. /// /// This type is used to keep

[PATCH] D128113: WIP: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3777353 , @mizvekov wrote: > @davrec @alexfh > > I finally managed to have a talk to @rsmith about this. > > He thinks that, even if we can't come up with a better solution in time, the > benefits of this patch

[PATCH] D127695: WIP: clang: Implement Template Specialization Resugaring

davrec added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:534-540 + QualType TransformSubstTemplateTypeParmType(TypeLocBuilder , + SubstTemplateTypeParmTypeLoc TL) { +QualType QT = TL.getType(); +const

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added inline comments. Comment at: clang/include/clang/AST/Type.h:5000 + + const TemplateTypeParmDecl *getReplacedParameter() const; + Another question worth raising: is it acceptable churn to change the return of `getReplacedParameter()` from a

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:1618 + QualType getSubstTemplateTypeParmType(QualType Replacement, +Decl *ReplacedDecl, +unsigned Index) const;

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

davrec accepted this revision. davrec added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130308/new/ https://reviews.llvm.org/D130308 ___ cfe-commits mailing list

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec accepted this revision. davrec added a comment. I hope I'm not stepping on any toes given the recent changes in code ownership, but I'm accepting this and D130308 because 1. They are a nice improvement to the AST that naturally follows from the earlier

[PATCH] D133262: [clang] Represent __make_integer_seq as alias template in the AST

davrec accepted this revision. davrec added a comment. LGTM aside from a nit Comment at: clang/include/clang/AST/DeclTemplate.h:455 + bool isTypeAlias() const; + Add doc, e.g. "Whether this is a written or built-in type alias template." And nit: maybe move

[PATCH] D127695: WIP: clang: Implement Template Specialization Resugaring

davrec added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:534-540 + QualType TransformSubstTemplateTypeParmType(TypeLocBuilder , + SubstTemplateTypeParmTypeLoc TL) { +QualType QT = TL.getType(); +const

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added a comment. This looks good, except for the confusion around the name `getReplacedDecl` - I would really like the public AST methods to be clearly named and documented, so please address that. Thanks for doing the performance tests. It may create some additional unique STTPTs,

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3753656 , @davrec wrote: > In D128113#3753640 , @mizvekov > wrote: > >> In D128113#3753624 , @davrec wrote: >> >>> Or just

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3753640 , @mizvekov wrote: > In D128113#3753624 , @davrec wrote: > >> Or just `SubstTemplateTypeParmType` could store this number in addition to >> its `TemplateTypeParmType`?

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. Great examples. > Check this example out: https://godbolt.org/z/rsGsM6GrM > > template struct A { > template struct B { > using type1 = void ((void (*...fps)(Ts, Us))); > }; > }; > using type2 = A::B::type1; > > TypeAliasDecl 0x55ffe8b45368

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. Two last thoughts: 1: To reiterate it doesn't seem right to view this (or any patch adding not-semantically-relevant info to the AST) as a one-size-fits-all costs vs. benefits optimization. On the one hand, the additional AST info hurts compilation performance. On the

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12767 +Ctx.getQualifiedType(Underlying), +::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl())); + } mizvekov wrote: > davrec wrote: > > mizvekov wrote: > > > davrec

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12767 +Ctx.getQualifiedType(Underlying), +::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl())); + } mizvekov wrote: > davrec wrote: > > This last argument should

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec added a comment. In D128113#3733764 , @mizvekov wrote: > In D128113#3733051 , @joanahalili > wrote: > >> We have a translation unit, on which we see an increase of compilation time >> and clang memory

[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

davrec added inline comments. Comment at: clang/unittests/AST/ASTContextParentMapTest.cpp:145 + EXPECT_THAT(Ctx.getParents(FrBLoc), ElementsAre(DynTypedNode::create(FrB))); + if (FrATagDecl) +EXPECT_THAT(Ctx.getParents(*FrATagDecl), sammccall wrote: > I'm

[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

davrec added a comment. In D131685#3716354 , @balazske wrote: > I really do not know why parent of the node for the owned `TagDecl` node is > the `FriendDecl` and not a `TypeLoc` node, but it is working. > The code `struct A { friend struct Fr; };`

[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

davrec added a comment. Once the FIXME is removed this looks good, but I was involved in this so better if @sammccall can give the thumbs up at least to the RecursiveASTVisitor code. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1577-1578

[PATCH] D131858: [clang] Track the templated entity in type substitution.

davrec added a comment. It was very good to separate this out, thanks. Can you can do some TMP performance testing, to verify the impacts are negligible before taking resugaring into consideration, to allay potential concerns? Comment at: clang/include/clang/AST/Type.h:1838

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12767 +Ctx.getQualifiedType(Underlying), +::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl())); + } This last argument should probably be omitted/nullptr passed,

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

davrec added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12218 +CTN, +NExpX && NExpY ? Optional(std::min(*NExpX, *NExpY)) : None); + } I'm not clear on how `NExpX` could not equal `NExpY` - could you add a test which demonstrates

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec added a comment. > The second paragraph is talking about 'Canonical nodes', not 'Canonical > types'. > > A canonical node is a type node for which 'isSugared' method returns false. Thanks for the clarification, but note that that term is not in general use so far as I'm aware. But

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

davrec added a comment. This part of the description is confusing: > We take two types, X and Y, which we wish to unify as input. > These types must have the same (qualified or unqualified) canonical > type. > > We dive down fast through top-level type sugar nodes, to the > underlying canonical

[PATCH] D127695: WIP: clang: Implement Template Specialization Resugaring

davrec added inline comments. Comment at: clang/test/Sema/Resugar/resugar-expr.cpp:244 +// N-error@-2 {{with an rvalue of type 'int'}} +} // namespace t21 Compositions of MemberExprs/CXXMemberCallExprs have an issue: ``` template struct A { struct Inner {

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

davrec accepted this revision. davrec added a comment. This revision is now accepted and ready to land. This corrects a genuine deficiency in the AST, and the patch LGTM. Can we knock this off Matheus' stack? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

davrec added inline comments. Comment at: clang/lib/AST/Expr.cpp:1949-1950 if (E->getCastKind() == CK_ConstructorConversion) return cast(SubExpr)->getConstructor(); I think the errors prove we should fall back to the most conservative possible

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

davrec added inline comments. Comment at: clang/lib/AST/Expr.cpp:1946-1947 for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) { SubExpr = skipImplicitTemporary(E->getSubExpr()); +SubExpr = SubExpr->IgnoreImplicit(); aaron.ballman wrote: >

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

davrec added a reviewer: rsmith. davrec added a subscriber: rsmith. davrec added a comment. In D117390#3246799 , @kimgr wrote: > @daverec I don't have commit access, could you help me land this? > > I've rebased on `main` without conflicts, and `ninja

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

davrec accepted this revision. davrec added a comment. This revision is now accepted and ready to land. Looks good, thanks for fixing this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

davrec added a comment. > There are already two way more sophisticated (forgive me my bias) > implementations in Clang that are for checking if two statements or decls are > the same. > > 1. ODRHash, used in modules to discover ODR violations > 2. ASTStructuralEquivalenceContext, used in

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

davrec added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372 +} + /// Determines whether two statement trees are identical regarding davrec wrote: > I would strongly support turning these functions into methods of

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

davrec added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59 + RTy->getCanonicalTypeUnqualified(); + + // FIXME: We should probably check the other kinds of nested name specifiers. steakhal wrote: >

[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

davrec added a comment. `throughUsingDecl` seems like a good solution, though I defer to regular ASTMatchers users. One other thought: can we add diagnostic notes using this new information, e.g. struct A1 { protected: using IntType = char; }; struct A2 { protected: using

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

davrec added a comment. A couple thoughts/cases to consider... Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:52 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left, const

[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

davrec accepted this revision. davrec added a comment. This revision is now accepted and ready to land. Looks great, thanks for identifying the need and banging this out so quickly. Hope you had some time to enjoy the holiday with your family! Dave Comment at:

[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

davrec added a comment. Looks good, a few notes. Comment at: clang/include/clang/AST/ASTContext.h:1559 + QualType getUsingType(const NamedDecl *Found, QualType Underlying) const; + I believe you mentioned ObjC considerations might require expanding beyond

[PATCH] D104182: [clang][NFC] Add IsAnyDestructorNoReturn field to CXXRecord instead of calculating it on demand

davrec added a comment. Was this performance hit when using the static analyzer? A quick search suggests `isAnyDestructorNoReturn()` is only called within the analyzer, whereas comparable CXXRecordDecl methods whose results are stored (`hasIrrelevantDestructor()` etc.) seem to be called

[PATCH] D101777: [clang] p1099 1/5: [NFC] Break out BaseUsingDecl from UsingDecl

davrec accepted this revision. davrec added a comment. Looks good, thanks for doing this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101777/new/ https://reviews.llvm.org/D101777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D102241: [clang] p1099 4/5: using enum EnumTag

davrec added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:520-523 + /// Like InstantiatedFromUsingDecl, but for using-enum declarations. Maps + /// from the instantiated using-enum to the templated decl from whence it + /// came. + llvm::DenseMap

[PATCH] D102241: [clang] p1099 4/5: using enum EnumTag

davrec added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:520-523 + /// Like InstantiatedFromUsingDecl, but for using-enum declarations. Maps + /// from the instantiated using-enum to the templated decl from whence it + /// came. + llvm::DenseMap

[PATCH] D101777: [clang] p1099 1/5: [NFC] Break out BaseUsingDecl from UsingDecl

davrec accepted this revision. davrec added a comment. Sorry for the delay. Richard should probably weigh in before absolutely committing to 1. BaseUsingDecl/UsingEnumDecl/UsingDecl (as implemented here) vs. single UsingDecl (Nathan's original approach) and 2. Renaming `getUsingDecl()` to