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