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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: dblaikie.
labath added a subscriber: dblaikie.
labath added a comment.

Lets loop in @dblaikie for start, and see how it goes from there. You can also check who modified/reviewed changes to this file lately..

I am a very big opponent of IsValid() methods, and so I am very much in favour of this idea. However, I expect it to be moderately controversial (which is part of the reason why I haven't tried proposing anything like that yet).

The thing I would like to see here is to have this behaviour be configurable via a traits argument of the Optional class, similarly to how DenseMap allows the type to specify default traits by specialising DenseMapInfo (I recommend taking a look at that for inspiration/consistency), but then a specific user can also override that and use a custom traits class. That would be particularly useful for builtin types -- one cannot pick any uint32_t value as an "invalid value" in general, but for a particular application, there usually is one free value. This would allow us to replace all the `pid == LLDB_INVALID_PROCESS_ID` checks in lldb with this new optional class.

Another thing to consider is that llvm::Optional tries to follow the interface of the (upcoming) std::optional, which does not have this configurability. This would put it in the same category as DenseMap (DenseOptional :P ?), which matches the interface (mostly) but has our own specific optimizations.

All that said, I don't think this needs to block your work on the python classes in lldb. IIUC, all/most of them are just temporaries anyway, and so their size does not matter that much (and can be often optimized away).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69230