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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked 4 inline comments as done.
Anastasia added inline comments.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8229
+  if (FTI.hasMethodTypeCVRUQualifiers()) {
+    FTI.MethodQualifiers->forEachCVRUQualifier(
         [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
----------------
rjmccall wrote:
> 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.
> Okay. Does forEachQualifier not visit address-space qualifiers?

It calls `forEachCVRUQualifier`, nothing else.

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

Added in FIXME.



================
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(
----------------
rjmccall wrote:
> "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?
Good point. Current implementation is generating one overload of each default/copy/move in generic address space only. But we could potentially look at changing this. If we could add extra overloads once we encounter each new ctor invocation it would be the easiest approach and this code would still be applicable. However, I would need to understand the feasibility in more details. May be this is something for the future work? Or do you have other suggestions?     


================
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: *
 
----------------
rjmccall wrote:
> Is there nothing in this test that's worth continuing to check while we work on fixing this problem?
We can only compile this file if we had address space qualifiers accepted on methods, but it's still WIP (https://reviews.llvm.org/D57464) and I don't have the time to fix it now. But at the same time the OpenCL test cover the functionality originally intended in this test. Not sure if it's better to remove this test completely?


================
Comment at: test/SemaCXX/address-space-ctor.cpp:11
+//expected-note at -6{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'MyType &&' for 1st argument}}
+//expected-note at -6{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
+//expected-note at -8{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
----------------
Not sure if we should change to:
  cannot be used to construct an object with '__attribute__((address_space(10)))'

Although for OpenCL it would be ok as is.

Or may be it's better to add custom printing of addr spaces?


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

https://reviews.llvm.org/D62156