mizvekov marked 5 inline comments as done. mizvekov added inline comments.
================ Comment at: clang/lib/AST/ASTContext.cpp:12218 + CTN, + NExpX && NExpY ? Optional<unsigned>(std::min(*NExpX, *NExpY)) : None); + } ---------------- davrec wrote: > I'm not clear on how `NExpX` could not equal `NExpY` - could you add a test > which demonstrates this case? It may not be needed in this patch, and it might in fact be related to a bug which I already solved in the main resugaring patch. I will double check later. ================ Comment at: clang/lib/AST/ASTContext.cpp:12767 + Ctx.getQualifiedType(Underlying), + ::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl())); + } ---------------- davrec wrote: > mizvekov wrote: > > davrec wrote: > > > mizvekov wrote: > > > > davrec wrote: > > > > > This last argument should probably be omitted/nullptr passed, since > > > > > # We probably won't encounter any owned tag decls in the types passed > > > > > to this function; > > > > > # Even if we did, it would not necessarily be true that they both > > > > > have non-null owned tag decls, and I don't see any nullptr checks in > > > > > getCommonDecl; and > > > > > # Even if we checked for nullptr there, and say passed the same > > > > > argument to X and Y so EX==EY, and that had an owned tag decl, it is > > > > > not clear to me it would be appropriate to construct a new type with > > > > > the same owned tag decl as another type. > > > > 1. We can see, a one liner like this will do: `int e = true ? (struct > > > > S*)0 : (struct S*)0;` > > > > > > > > Though normally in an example like the above, you would only see the > > > > owned decl on X, because when we get to Y the name has already been > > > > introduced into the scope. > > > > > > > > I have an idea for a test case, but it's a bit convoluted: > > > > > > > > This also introduces an OwnedTagDecl: `(struct S<T>*)0`. > > > > > > > > So with resugaring, if we resugar T, it might be possible to construct > > > > this situatiation. Given if it's appropriate to keep the OwnedTagDecl > > > > when resugaring, of course, which goes to 3) > > > > > > > > 2. This is handled by `declaresSameEntity`, if either or both decls are > > > > null, we say they are not the same decl. > > > > > > > > 3. That I am not sure. I see that this OwnedTagDecl is only used by the > > > > type printer, and it replaces printing the rest of the type by printing > > > > the OwnedDecl instead. So why do you think it would not be appropriate > > > > that the rebuilt type keeps this? > > > (Re #3) Because I think the sense ownership is respected in how > > > ownedTagDecls are presently handled: at most one ElaboratedType owns a > > > particular TagDecl, *and* that type does not seem to be reused elsewhere > > > in the AST. (This is relied on in https://reviews.llvm.org/D131685, > > > which was on my mind when I looked at this.) > > > > > > E.g. consider this expansion of your example: > > > ``` > > > auto e = true ? (struct S*)0 : (true ? (struct S*)0 : (struct S*)0); > > > ``` > > > The first ElaboratedType has an ownedTagDecl; the second is a distinct > > > ElaboratedType without an ownedTagDecl, and the third equals the second. > > > > > > Now in practice what this means is that `getCommonDecl` when used on > > > binary expressions of this sort will always be nullptr, so no harm done. > > > But suppose it is called with X=Y=some ET with an ownedTagDecl (which is > > > the only time I believe we could see a non null getCommonDecl result > > > here). Should the type that is returned have that same owned tag decl? > > > I think that would violate the sense of the original type "owning" that > > > decl that seems to be respected elsewhere in the AST. > > > > > > Re type printing, I believe that ownedTagDecls only affects printing of > > > defined TagDecls like `struct S { int i; } x;` (which is what I was > > > mostly thinking re ownedTagDecls - I don't think we will see those here); > > > for undefined ones like `struct S x;`, printing should be unaffected > > > whether there is an ownedTagDecl or not. > > > > > With type deduction this can happen: > > > > ``` > > auto x = (struct S*)0; // it will also appear in the type of x > > using t = decltype(x); // and now also in t > > ``` > > > > With your second example, it can also happen: > > > > ``` > > struct S { int i; } x; > > int e = true ? &x : (struct S*)0; > > ``` > > > > In those cases, the type node that owns the decl will be the same object, > > but that is only because of uniquing. > > > > It may be possible that two different objects end up in this situation, if > > for example they come from different modules that got merged. > > With type deduction this can happen: > > ``` > > auto x = (struct S*)0; // it will also appear in the type of x > > using t = decltype(x); // and now also in t > > ``` > > My local clang is out of date - the type of `x` for me is just an AutoType > wrapping a PointerType to a RecordType. In light of the addition of the > ElaboratedType there, it seems fine to be consistent here with however that > case handles ownedTagDecls, but FWIW I do not think that deduced > ElaboratedType should have an ownedTagDecl either. Only the original type > should be the owner - not any type deduced from it. > > This is a minor issue with few if any observable effects for now, but should > be kept in mind if issues arise later: the fact the ownership of > ownedTagDecls is respected was the only factor that made > https://discourse.llvm.org/t/ast-parent-of-class-in-a-friend-declaration/64275 > easily solvable. > > > With your second example, it can also happen: > > > > ``` > > struct S { int i; } x; > > int e = true ? &x : (struct S*)0; > > ``` > > The DeclRefExpr `x` indeed reuses the ElaboratedType used in the VarDecl `x`, > with its ownedTagDecl and all. That seems fair, a special case. For all > other type deductions I would mildly object to including an ownedTagDecl. > I think that assumption might work on ElaboratedTypeLocs instead. I think only one per TU should appear in the AST. Resugaring might make us rebuild such TypeLocs, but the non-resugared one should be unreachable from the AST. ================ Comment at: clang/lib/AST/ASTContext.cpp:12872 } SplitQualType SX = X.split(), SY = Y.split(); ---------------- davrec wrote: > It would be very helpful to incorporate your description and the description > from D111283 as comments in this function. E.g. something like the following > ... Thanks! I also added some extras in there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130308/new/ https://reviews.llvm.org/D130308 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
