Codebases that need to be compatible with the Microsoft ABI can pass
this flag to avoid issues caused by the lack of a fixed ABI for
incomplete member pointers.
Details
Details
Diff Detail
Diff Detail
- Repository
- rC Clang
Event Timeline
Comment Actions
Should we do this in exactly the places we would lock in an inheritance model in the MS ABI, i.e. when the member pointer type is required to be complete? I think we could take this code:
bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
TypeDiagnoser *Diagnoser) {
// FIXME: Add this assertion to make sure we always get instantiation points.
// assert(!Loc.isInvalid() && "Invalid location in RequireCompleteType");
// FIXME: Add this assertion to help us flush out problems with
// checking for dependent types and type-dependent expressions.
//
// assert(!T->isDependentType() &&
// "Can't ask whether a dependent type is complete");
// We lock in the inheritance model once somebody has asked us to ensure
// that a pointer-to-member type is complete.
if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
if (!MPTy->getClass()->isDependentType()) {
(void)isCompleteType(Loc, QualType(MPTy->getClass(), 0));
assignInheritanceModel(*this, MPTy->getMostRecentCXXRecordDecl());
}
}
}And rewrite the side-effecting isCompleteType call to use RequireCompleteType with the warning diagnostic. I guess we would change the isMicrosoft check to alternatively check if the diagnostic is enabled. It still has the problem that enabling a warning can cause template instantiation errors, but oh well.
Comment Actions
We should keep the isCompleteType call and diagnose if (!isCompleteType(...)). RequireCompleteType needs an error diagnostic. (In particular, RequireCompleteType will produce an error suggesting a module import if a module containing the definition is known but not imported; isCompleteType will just treat the type as incomplete in that case. Don't RequireCompleteType unless you require a complete type :) )
| clang/lib/Sema/SemaType.cpp | ||
|---|---|---|
| 2451–2455 ↗ | (On Diff #149001) | This doesn't seem right. Calling RequireCompleteType will trigger instantiation of the type if it's templated, which can affect the validity (and rarely, the meaning) of the program. Also, passing a warning diagnostic into RequireCompleteType doesn't actually work -- there are cases where it will disregard your diagnostic and substitute one of its own, which will be an error. |
| clang/lib/Sema/SemaType.cpp | ||
|---|---|---|
| 2451–2455 ↗ | (On Diff #149001) | You're right, but I couldn't see a way of testing whether a type is complete without triggering those side effects. It does look like we can at least avoid some of them with isCompleteType, though. I guess the best we can do is to move the diagnostic into RequireCompleteTypeImpl and make it conditional on isCompleteType as @rnk suggested. |
| clang/lib/Sema/SemaType.cpp | ||
|---|---|---|
| 7599–7604 ↗ | (On Diff #149017) | It's not reasonable to call isCompleteType here; this is non-conforming behavior, and it's not reasonable for a warning flag to change the semantics or interpretation of the program. (We actually rely on this in several ways -- -Weverything is not supposed to change the validity of programs, and reuse of implicit modules with different warning flags relies on the property that the compiler behavior is as if we compile with -Weverything and then filter the warnings after the fact.) It seems fine to do this in the MS ABI case, since we will attempt to complete the type anyway in that mode. If you want to apply this more generally, I think there are two options: either add a -f flag to carry this semantic change, or figure out whether the type is completable without actually completing it (for example, check whether it's a templated class whose pattern is a definition). |
| clang/lib/Sema/SemaType.cpp | ||
|---|---|---|
| 7599–7604 ↗ | (On Diff #149017) | That all seems reasonable. I think the right thing to do here is to add a -fcomplete-member-pointers flag then -- couple of reasons:
|