[Lldb-commits] [PATCH] D69230: RFC: specialized Optional<T> for T that can represent its own invalid state

Thu Oct 24 12:12:45 PDT 2019

dblaikie added a comment.

In D69230#1720255 <https://reviews.llvm.org/D69230#1720255>, @labath wrote:

> In D69230#1720246 <https://reviews.llvm.org/D69230#1720246>, @dblaikie wrote:
>
> > In D69230#1720048 <https://reviews.llvm.org/D69230#1720048>, @labath wrote:
> >
> > > That said, I think you have convinced me that having different optional representations for a single type is not a good idea. It's probably better to use some form of a "strong" typedef to achieve that instead.
> >
> >
> > I've not followed this part of the thread properly - could you/someone rephrase the concerns here?
>
>
> Sure. lldb currently has types like lldb::pid_t, and uses identifiers like LLDB_INVALID_PID for the "None" value. I wanted to define pid_t as something like `Optional<some_integer_type, PIDInfo>`, where PIDInfo would specify what is the representation of an "invalid" pid. The reason I abandoned this idea was that this arrangement makes it impossible to represent (in the type system) a pid which is always valid. I now think that it would be better (at least for this use case) to have `pid_t be a "strong" typedef of the integer type. At that point you can specify an "invalid" value for this type even without a separate Optional template argument...

Ah, thanks - yeah, agreed. The invalid state should always be hidden/non-public - though I'm not sure that's quite true for DenseMap/Set/etc - yeah, the DenseMapInfo traits use ~0 and ~0-1 as the empty and tombstone values for integer types.

So I think we should generalize DenseMapInfo traits (or perhaps build DenseMapInfo traits, that need two states, on top of the OptionalInfo trait that only needs one) into some generalized "I'd like this many bits of state" & yeah, probably a default trait argument to Optional - and, yes, I agree that it's generally better to provide a wrapper type that excludes the invalid states from being publicly accessible, but I guess folks have found it useful to be able to have a DenseSet<int> without worrying too much about the fact they can't store the max int in that set (because it's the empty value).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69230