Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 3: (1 comment) Just responding to a comment on patch set 3. I'll review the new patch set later. http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 26: CHECK_OK > Defining macros for checking statuses like this is fairly common practice, Yeah, we do often use macros for things like verifying status objects but typically macros are best to avoid unless there's a good reason to use them. RETURN_IF_ERROR() is kind of a useful macro due to it actually returning control flow from the current function on an error (consider the alternative). However, something like EXIT_IF_ERROR (which is basically what you have here) isn't really providing much beyond what a similar function is since the control flow at the point of calling it doesn't need to branch differently. Then I think we typically only really provide EXIT_IF_ERROR to be consistent with RETURN_IF_ERROR, but we don't have those here so it feels out of place. If anything though, macro code is very error prone and hard to read. Anything that compromises readability should be avoided unless there's a good reason to do so :) -- To view, visit http://gerrit.cloudera.org:8080/2645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0 Gerrit-PatchSet: 3 Gerrit-Project: hs2client Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-HasComments: Yes
