Thanks for the review Richard. I believe I've addressed your comments. PTAL
================
Comment at: lib/Sema/SemaCUDA.cpp:110-111
@@ +109,4 @@
+ bool ConstRHS) {
+ CUDAFunctionTarget InferredTarget;
+ bool HasInferredTarget = false;
+
----------------
rsmith wrote:
> `Optional<CUDAFunctionTarget>` InferredTarget;
Done.
================
Comment at: lib/Sema/SemaCUDA.cpp:119
@@ +118,3 @@
+ // Infer the target of this member base on the ones it should call.
+ for (const auto &B : ClassDecl->bases()) {
+ const RecordType *BaseType = B.getType()->getAs<RecordType>();
----------------
rsmith wrote:
> For a non-abstract class, you should visit virtual bases as well as direct
> bases. (For an abstract class, you should skip direct and indirect virtual
> bases.)
Done.
================
Comment at: lib/Sema/SemaCUDA.cpp:128-132
@@ +127,7 @@
+ LookupSpecialMember(BaseClassDecl, CSM,
+ /* ConstArg */ ConstRHS,
+ /* VolatileArg */ false,
+ /* RValueThis */ false,
+ /* ConstThis */ false,
+ /* VolatileThis */ false);
+
----------------
rsmith wrote:
> rsmith wrote:
> > These are not necessarily correct; the user might have explicitly defaulted
> > a const/volatile/whatever special member function.
> Hmm, I see you're not actually doing this for any defaulted function, just
> for an implicit one. In that case, this looks fine, but please rename the
> function to talk about implicit special members not defaulted ones.
Renaming done.
================
Comment at: lib/Sema/SemaCUDA.cpp:146-148
@@ +145,5 @@
+ if (ResolutionError) {
+ Diag(ClassDecl->getLocation(),
+ diag::err_implicit_member_target_infer_collision)
+ << (unsigned)CSM << InferredTarget << BaseMethodTarget;
+ return;
----------------
rsmith wrote:
> eliben wrote:
> > rsmith wrote:
> > > Declaring implicit special members is done lazily, so emitting
> > > diagnostics from here will result in erratic behavior. It would be better
> > > to defer the diagnostic until the special member is defined in C++98, and
> > > to mark the member as deleted in this case in C++11.
> > If I just mark the member as deleted in C++11, is there away to still emit
> > the descriptive diagnostic? Just deleting the member doesn't provide the
> > user with any hint of what the underlying problem is.
> Yes, see `Sema::NoteDeletedFunction`.
Thanks. I ended up implanting this into ShouldDeleteSpecialMember, with
diagnostics; this is called from NoteDeletedFunction, and it also takes care of
actually deleting the member in C++11 mode. Let me know if this makes sense.
================
Comment at: lib/Sema/SemaOverload.cpp:5637
@@ -5636,3 +5636,3 @@
if (const FunctionDecl *Caller = dyn_cast<FunctionDecl>(CurContext))
- if (CheckCUDATarget(Caller, Function)) {
+ if (!Caller->isImplicit() && CheckCUDATarget(Caller, Function)) {
Candidate.Viable = false;
----------------
rsmith wrote:
> eliben wrote:
> > rsmith wrote:
> > > Hmm, why do you need this change?
> > Consider what happens when the target inference method runs. It looks up
> > the ctors of base classes, which gets to this point. Now, at this point we
> > don't yet know what the caller's (the implicit method being defined) target
> > is, so we can not reason whether there's a target mismatch.
> >
> > AFAIU this added test solves the problem, since we really cannot reject
> > candidates based on target-ness when declaring an implicit member. The
> > implicit member's target-ness will be determined by the inference method,
> > and the collision test (the one with the Diag you commented on) should take
> > care of mismatches.
> >
> > Does this make sense?
> Thanks, that makes sense. This at least deserves an explanatory comment.
Comment added, let me know if you'd want to see more details in it.
http://reviews.llvm.org/D5199
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
