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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
