Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11693 )
Change subject: Fix thrift operator< implementations ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/11693/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11693/4//COMMIT_MSG@7 PS4, Line 7: Fix thrift operator< implementations Not related to this commit per se, but how do you feel about the fact that these operators go against the style guide's recommendation of only overloading operators on "your own" types? See https://google.github.io/styleguide/cppguide.html#Operator_Overloading. http://gerrit.cloudera.org:8080/#/c/11693/4/src/kudu/sentry/thrift_operators-test.cc File src/kudu/sentry/thrift_operators-test.cc: http://gerrit.cloudera.org:8080/#/c/11693/4/src/kudu/sentry/thrift_operators-test.cc@51 PS4, Line 51: ::sentry::TSentryRole role_a; I left similar feedback on Hao's patches, but could we add "using" statements here to make this less verbose? The common TSentry prefix in these types makes it pretty easy IMHO to tell that they originate in Sentry and not Kudu. http://gerrit.cloudera.org:8080/#/c/11693/4/src/kudu/sentry/thrift_operators-test.cc@72 PS4, Line 72: // TSentryPrivilege::operator< As written, it seems that this test and the test for TSentryAuthorizable hinge on the fact that serverName and dbName are the same, while tableName either exists or doesn't. Meaning, the 'interesting' part is that in one object the optional tableName (or table) is set, and in the other it isn't. However, if I accidentally change the serverName (or dbName) string constant in one object so that the field no longer matches across both objects, the test doesn't fail, but it loses the coverage ensuring that the optionality behavior in operator< is correct. To help protect against this, you could put the string constants in variables and reference the variables. That'll make it a little harder to mess up the test in this way. http://gerrit.cloudera.org:8080/#/c/11693/4/src/kudu/sentry/thrift_operators-test.cc@86 PS4, Line 86: Nit: got an extra empty line here. -- To view, visit http://gerrit.cloudera.org:8080/11693 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1aacf602c603b05433c357a4a236ba0b9e521392 Gerrit-Change-Number: 11693 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 17 Oct 2018 04:20:29 +0000 Gerrit-HasComments: Yes
