[PATCH] D99561: Support visitor pattern by PointerUnion.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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?

> - 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?

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99561