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@-6{{candidate constructor (the implicit move constructor) not
viable: no known conversion from 'int' to 'MyType &&' for 1st argument}}
+//expected-note@-6{{candidate constructor ignored: cannot be used to construct
an object in address space '__attribute__((address_space(10)))'}}
+//expected-note@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
