lordgamez commented on PR #2261: URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5640119846
> Since the clear function operates with UA_NodeId, I would treat the internals as a black box for the purpose of cleanup, and always call UA_NodeId_clear on it, regardless of the union values. I'd treat UA_NodeId as a resource handle and UA_NodeId_clear its corresponding resource cleanup function, and resource handles can be owning or non-owning, that's why I think we should use gsl::owner. I agree the internals should be treated as a black box. One of the reasons for this PR is that we had previously not tested node IDs with string-based underlying types, which is why the issue didn't show up in our memchecks. The fix now handles all union type cases the same way, clearing them every time, even though for integer node ID types the clear operation is a no-op. That said, I still don't understand how gsl::owner could be used here. In the NodeId RAII wrapper, ownership is taken at construction and held until the end of its lifetime, so ownership never changes over the course of this solution. Also, the `id_` member is of type `UA_NodeId`, not `UA_NodeId*`, and `gsl::owner` can only be applied to a pointer type. In every function where we actually return a pointer, we deliberately return a non-owning one, since `NodeId` retains ownership of the `id_` member itself. Maybe I'm missing something, could you elaborate on how you pictured using `gsl::owner` 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]
