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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

(I didn't mean to submit the last comment before I finished the review, sorry)

It looks like you've chosen to treat this as a DR that we should apply under all standards.  Have you investigated whether it causes compatibility problems?  It does look like it'd be painful to try to support both rules.



================
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()) &&
----------------
Is it okay to check them in sequence like this, or do we need to check in advance which case to use?


================
Comment at: clang/lib/Sema/SemaAccess.cpp:983
+      }
+      return false;
+    };
----------------
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?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:2296
+      }
+    }
+
----------------
I wonder under this new model if we can record that we saw something along ambiguous paths and avoid a lot of work in access-checking in the common case where we don't?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94987