[PATCH] D62156: [Sema][PR41730] Diagnose addr space mismatch while constructing objects

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3653
+def note_ovl_candidate_illegal_constructor_adrspace_mismatch : Note<
+    "candidate constructor ignored: address space mismatch between object and constructor">;
 def note_ovl_candidate_bad_deduction : Note<
----------------
"candidate constructor ignored: cannot be used to construct an object in address space %0"?


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8229
+  if (FTI.hasMethodTypeCVRUQualifiers()) {
+    FTI.MethodQualifiers->forEachCVRUQualifier(
         [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
----------------
Anastasia wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > We want to catch `_Atomic`, too, so please just change this loop to ignore address-space qualifiers, using a flag to decide whether to call `setInvalidType`.
> > If there aren't any qualifiers we're skipping, the flag isn't necessary.
> We are skipping addr space currently. I use this flag to avoid setting declaration as invalid below if it's only qualified by an addr space.
Okay.  Does `forEachQualifier` not visit address-space qualifiers?

Please leave a comment explaining that that's the intended behavior: we should visit everything except an address space.


================
Comment at: lib/Sema/SemaOverload.cpp:6095
+    // Check that addr space of an object being constructed is convertible to
+    // the one ctor qualified with.
+    if (!Qualifiers::isAddressSpaceSupersetOf(
----------------
"Check that the constructor is capable of constructing an object in the destination address space."

Should there be an exception here for trivial default/copy/move constructors?


================
Comment at: test/CodeGenCXX/address-space-of-this.cpp:3
 // RUN: %clang_cc1 %s -std=c++17 -triple=spir -emit-llvm -o - | FileCheck %s
+// XFAIL: *
 
----------------
Is there nothing in this test that's worth continuing to check while we work on fixing this problem?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62156/new/

https://reviews.llvm.org/D62156