rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Looks good. Some possible improvements to make the diagnostic a bit more
precise, but I'd be happy if you want to address some of those as follow-ups
after this change.
================
Comment at: clang/lib/Sema/SemaCast.cpp:1041
+ ASTContext &Context) {
+ if (SrcType->isPointerType() && DestType->isPointerType())
+ return true;
----------------
I think we should also treat references like pointers here, and similarly for
block pointers and ObjC pointers. Perhaps using `hasPointerRepresentation()`
would make sense?
================
Comment at: clang/lib/Sema/SemaCast.cpp:1044-1048
+ // Allow integral type mismatch if their size are equal.
+ if (SrcType->isIntegralType(Context) && DestType->isIntegralType(Context))
+ if (Context.getTypeInfoInChars(SrcType).Width ==
+ Context.getTypeInfoInChars(DestType).Width)
+ return true;
----------------
ychen wrote:
> rsmith wrote:
> > In addition to allowing the cases where the sizes are the same width,
> > should we also allow cases where the promoted types are the same width (eg,
> > `int` versus `short`)? What does GCC do?
> GCC does exactly that. I didn't find a way to do that by looking at
> TargetInfo. Maybe there is a way for clang?
Hm. The right thing to do would be to call
`ABIInfo::isPromotableIntegerTypeForABI`, but that's defined in `CodeGen` so we
can't call it from here without violating layering. I suppose we could pull it
up into the target info in `Basic`, if you're prepared to do that kind of
refactoring, but maybe for now we can skip this check and emit a few warnings
that GCC doesn't.
================
Comment at: clang/lib/Sema/SemaCast.cpp:1050-1055
+ llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+ StructuralEquivalenceContext Ctx(
+ Context, Context, NonEquivalentDecls, StructuralEquivalenceKind::Default,
+ false /*StrictTypeSpelling*/, false /*Complain*/,
+ false /*ErrorOnTagTypeMismatch*/);
+ return Ctx.IsEquivalent(SrcType, DestType);
----------------
ychen wrote:
> rsmith wrote:
> > I think a "same type" check (`Context.hasSameUnqualifiedType(SrcType,
> > DestType)`) would be more appropriate here than a structural equivalence
> > check.
> Agreed.
Taking this a bit further: we could use `hasSimilarType` to allow changes in
nested cv-qualifiers. But given that we've already allowed arbitrary
differences in pointee types, I suppose this would only affect
pointers-to-members, and it might be preferable to allow arbitrary changes
between pointer-to-member types here (except in the MS ABI case where there are
meaningful differences in pointer-to-member representations between classes).
What does GCC do with differences in pointer-to-member types in function
parameter types?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97831/new/
https://reviews.llvm.org/D97831
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
