[PATCH] D99561: Support visitor pattern by PointerUnion.

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D99561#2672679 <https://reviews.llvm.org/D99561#2672679>, @dblaikie wrote:

> In D99561#2670269 <https://reviews.llvm.org/D99561#2670269>, @scott.linder wrote:
>
>> To chime in on the spelling of things relative to `std`, I had similar questions for IntrusiveVariant in https://reviews.llvm.org/D98477
>>
>> A natural question that might help with deciding how to handle `visit` is: how should we spell alternative type access? `std` uses the free function `std::get`. It makes sense, as that same free function is overloaded for e.g. `std::tuple`, `std::pair`, `std::variant`, etc. but we don't currently have anything similar in ADT.
>>
>> I think my leaning would be to go all-in on the `std` approach by doing the following:
>>
>> - Add an `llvm::adt` namespace (or something similar) containing things which are "designed to fulfill the same roles as the identically-named member of the `std` namespace, but for whatever reason we cannot actually use that member directly". Generally the reason will be: that thing is not an extension point, and it is forbidden to re-open `std` to overload that thing.
>
> Oh - what would happen if the name went in llvm directly and we called it via adl? ie: made the call unqualified & let ADL+overload resolution sort it out?

I hadn't thought too much about using ADL, as pre-C++20 there is a subtle issue with ADL when the function is a template, see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0846r0.html

I may have dismissed it too quickly, though, as either `using llvm::get;` or `using namespace llvm;` or `using namespace std;` should be enough to get ADL to work fine for bare `get<N>()` pre-C++20.

Also, in trying to find other examples of people doing this, it seems like C++17 makes `get` a customization point for at least one case: https://en.cppreference.com/w/cpp/language/structured_binding (under case 2, it reads "Otherwise, get<i>(e), where get is looked up by argument-dependent lookup only, ignoring non-ADL lookup.")

>> - Add the appropriate `llvm::adt::get(llvm::PointerUnion)` overloads
>> - When adding `InrusiveVariant`, I can add `llvm::adt::get(llvm::IntrusiveVariant)` overloads
>> - Add `llvm::adt::visit` and the appropriate `llvm::PointerUnion` overloads
>> - When adding `IntrusiveVariant`, I can add `llvm::adt::visit` overloads
>>
>> Then everything is spelled the same/quacks the same, but names are not ambiguous in the context of `using namespace llvm;` or `using namespace std;` or both.
>
> Are there cases where they'd be ambiguous, though? Wouldn't template ordering still match them distinctly, unless one or more of these templates were overly general?

I think you're right, I had just assumed there would be an issue for some reason.

>> For other things, where we expect to delete our copy entirely once we move the codebase to a certain `std` version, we can just drop the exact "polyfill" definition with the same name into `llvm::`, and then delete the code later. An example I plan to post a patch for is `in_place{_type,_index}_t`.
>>
>> What are people's thoughts?
>>
>> Edit: Alternatively the additional namespace I mention could be omitted, and every use of e.g. `get` would have to be qualified with either `std::` or `llvm::` if it would be ambiguous. My intention was to try to make it obvious that the `adt::` version is definitely not //actually // the `std::` version, even with the most common forms of `using namespace`.
>
> I'm not sure to me it's important that they be obviously distinct - if they do the same thing I'm OK with it being a bit vague. Like 'swap' called via the adl (admittedly that one's very well documented as being adl) - overload resolution'll work itself out and if the behavior's obvious enough I don't think it'll matter to most folks whether it's implemented by the standard or llvm.

That seems reasonable to me, I was just cautious to use ADL for an unqualified name found in `std` when the standard doesn't explicitly "endorse" it, although I'm not sure why; I don't think there is anything forbidding us from doing it.

I think I'm just cautious around ADL as a feature because it can lead to confusion if we aren't careful, and as I linked above it has subtley different behavior for templates pre-C++20. As you mention, our `get` is faithful to the semantics of `std::get`, so it seems like a good candidate for ADL.



================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:307
+  /// Named differently because the calling convention is different. Name is
+  /// inspired by Rust's `match` keyword.
+  template <typename... LAMBDAS> auto match(LAMBDAS... lambdas) {
----------------
I don't know that I'm in favor of having this version at all, but even if we do I don't know that comparing it to Rust's `match` helps explain it. I see Rust's `match` as closer to `std::visit`, in that neither use the postfix-dot notation.

Naming aside, I also just think the bar for ergonomics needs to be higher to justify having more than one way to do something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99561