Page MenuHomePhabricator

[ADT][WIP] Proof of concept impl of generic visit for PointerUnion
AcceptedPublic

Authored by scott.linder on May 26 2021, 9:16 PM.

Details

Summary

If the parent patch (or something similar) is accepted, then this is equivalent to https://reviews.llvm.org/D99561 and could supercede it, possibly with the addition of the match convenience method.

Diff Detail

Event Timeline

scott.linder created this revision.May 26 2021, 9:16 PM
scott.linder requested review of this revision.May 26 2021, 9:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 9:16 PM
scott.linder edited the summary of this revision. (Show Details)May 26 2021, 9:18 PM
scott.linder added reviewers: mehdi_amini, dblaikie.
dblaikie added inline comments.Jun 3 2021, 7:57 PM
llvm/include/llvm/ADT/PointerUnion.h
284

Is this the only reason the variant traits needs to be befriended by the PointerUnion? If so, perhaps this API could be exposed in PointerUnion's public API rather than using friendship?

scott.linder added inline comments.Jun 4 2021, 9:45 AM
llvm/include/llvm/ADT/PointerUnion.h
284

From a pure perspective I think it is an implementation detail, which is the same reason I didn't expose the index in IntrusiveVariant either. It seems reasonable to expose, as std::variant does expose an ::index method, but I can't think of many uses of it that don't have a better, type-safe alternative.

dblaikie accepted this revision.Jun 4 2021, 1:21 PM
dblaikie added inline comments.
llvm/include/llvm/ADT/PointerUnion.h
284

Eh, I don't feel too strongly about it - but figure if it's an important feature for VariantTraits, that seems like as good a reason as any for it to be public (since you can access the property via variant traits anyway - so why not access it directly at that point?).

Up to you.

This revision is now accepted and ready to land.Jun 4 2021, 1:21 PM
bakhtiyarneyman accepted this revision.Aug 29 2021, 2:00 PM