Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11720 )
Change subject: [sentry] improve SentryAction ...................................................................... Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/11720/2/src/kudu/sentry/sentry_action.h@66 PS2, Line 66: static Status FromString(const std::string& str, Action* action); > Yes, Andrew suggested to add UNINITIALIZED enum to avoid undefined behavior On second thought, my suggestion (to remove UNINITIALIZED and the default constructor) will only work if FromString() is more frustrating to use, either in the way that you've done here, or by forcing the second parameter SentryAction to be wrapped in a unique_ptr. Neither of those solutions are particularly elegant, so I think you should ignore my suggestion, restore UNINITIALIZED, and restore the default constructor. Then the second parameter can be a SentryAction* (though I still think it's clearer for FromString to be a static factory method of that kind than a mutator). -- To view, visit http://gerrit.cloudera.org:8080/11720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icc6c0fb2a33a24745936c96363390e776b24513f Gerrit-Change-Number: 11720 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 19 Oct 2018 16:49:45 +0000 Gerrit-HasComments: Yes
