[PATCH] D94987: DR39: Perform ambiguous subobject checks for class member access as part of object argument conversion, not as part of name lookup.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D94987#2507481 <https://reviews.llvm.org/D94987#2507481>, @rjmccall wrote:

> How does this new rule work if we find overlapping but non-equal sets of declarations in different subobjects?  I'm sure you can get that with `using` declarations.

You can. The rule is that you resolve using-declarations to the declarations they name, and you resolve typedef declarations to the types they name, and the resulting set of declarations and types must be the same in every subobject in which there are (non-shadowed) results -- if not, the lookup is ill-formed. (Put another way, the set of entities found in each subobject must be the same, but the set of declarations found in each subobject can differ.)



================
Comment at: clang/include/clang/AST/CXXInheritance.h:77
 
-  CXXBasePath() = default;
+  /// Additional data stashed on the base path by its consumers.
+  union {
----------------
rjmccall wrote:
> Is there a way to know which case we're in, or do different consumers do different things?
We could use a discriminated union here. Ultimately, I think what we really want is to provide some opaque storage for the consumer to use, rather than to have some hard-coded list here. I haven't thought of a good way to expose that; I don't think it's worth templating the entire base path finding mechanism to add such storage, and a single pointer isn't large enough for a `DeclContext::lookup_result`.

Vassil's https://reviews.llvm.org/D91524 will make the `lookup_result` case fit into a single pointer; once we adopt that, perhaps we can switch to holding an opaque `void*` here?


================
Comment at: clang/lib/Sema/SemaAccess.cpp:891
+    // Note that declaresSameEntity doesn't correctly determine whether
+    // two type declarations declare the same type entity.
+    if ((!Best || Best->getAccess() > Corresponding->getAccess()) &&
----------------
rjmccall wrote:
> Is it okay to check them in sequence like this, or do we need to check in advance which case to use?
For types, `declaresSameEntity` returns true for a subset of the cases for which `declaresSameType` returns true, so it will always be correct to check both (though for a type, checking `declaresSameEntity` will always be redundant).


================
Comment at: clang/lib/Sema/SemaAccess.cpp:983
+      }
+      return false;
+    };
----------------
rjmccall wrote:
> Is this not doing a *lot* of extra work?  I suppose this is only in the slow path where we weren't able to immediately recognize that the found decl is public or we're in a scope with obviously adequate access?
Compared to the `isDerivedFrom` check, this will be doing one extra decl context hash table lookup per class we consider on each path. I haven't measured the extra work here; I can if you're concerned.

I thought for a while about better ways to handle this, and I'm not sure there's a good middle ground between redoing the work like this and saving the base paths from name lookup and using them here. If you have better ideas, I'd appreciate them!

I suppose we probably have a spare bit on `DeclAccessPair` that we could use to track whether we're in the "hard" (found in classes of different types) case, and we could use `isDerivedFrom` in the "easy" cases. Do you think that's worthwhile?

Ah, I see you suggest doing something like that below. Yes, I can give that a go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94987