ellutionist commented on PR #1418: URL: https://github.com/apache/incubator-kvrocks/pull/1418#issuecomment-1537156966
> > The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional. > > Sure, you can use something like `std::optional` or `std::unique_ptr`. > > > Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash. > > I think in kvrock, we use `std::shared_ptr` if and only if the object need to be **"shared"** between multiple references, and the lifetime of this object cannot be determined **statically** (e.g. given an object `o` and two reference `a` and `b`, the lifetime of `o` is equal to the maximum of lifetime of `a` and `b`, which need to be determined **dynamically**). > > So currently, just _"in the future ..."_ seems not to be an accredited reason to me, since if this reason holds, we may need to change many raw pointer in the codebase to `shared_ptr`. If you just need a non-owning pointer (or called an observer/view/span, in modern C++), raw pointer is currently the available choice, since [`std::observer_ptr`](https://en.cppreference.com/w/cpp/experimental/observer_ptr) or `std::optional<T&>` (optional with reference type) is not in the standard. Thank you. I got the point of the difference between dynamic and static lifetime. From this point of view the `share_ptr` is not proper in this case. Perhaps the raw pointer is the best solution here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
