[PATCH] D47503: Sema: Add a warning for member pointers with incomplete base types.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:7599-7604
+      if (!Context.getDiagnostics().isIgnored(diag::warn_memptr_incomplete,
+                                              Loc) &&
+          !MPTy->getClass()->getAsCXXRecordDecl()->isBeingDefined() &&
+          !isCompleteType(Loc, QualType(MPTy->getClass(), 0)))
+        Diag(Loc, diag::warn_memptr_incomplete)
+            << QualType(MPTy->getClass(), 0);
----------------
rsmith wrote:
> 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).
That all seems reasonable. I think the right thing to do here is to add a `-fcomplete-member-pointers` flag then -- couple of reasons:
- a user of this feature would probably expect to see diagnostics in the case where `RequireCompleteType` would fail in MS mode;
- I'm planning to use the extra semantic information that would result from turning this on in IRgen when a new `-fsanitize=cfi-*` flag is enabled, which would mean that we would actually need to call `RequireCompleteType` at Sema time. I was thinking about tying the extra `RequireCompleteType` calls to the new `-fsanitize=cfi-*` flag, but that seems somewhat questionable as well (one of the main reasons is that it would be part of the `-fsanitize=cfi` group, and I don't want to make `-fsanitize=cfi` non-conforming), so we can probably get away with just a recommendation that `-fcomplete-member-pointers` is used with `-fsanitize=cfi`.


https://reviews.llvm.org/D47503