Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20320 )
Change subject: [consensus] clean-up on OpId utilities ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/20320/1/src/kudu/consensus/opid_util.h File src/kudu/consensus/opid_util.h: http://gerrit.cloudera.org:8080/#/c/20320/1/src/kudu/consensus/opid_util.h@58 PS1, Line 58: inline bool operator>(const OpId& lhs, const OpId& rhs) { > Also, why not use the negation of the < operator above? Probably, you meant the negation of the '<=' operator? I just moved the code from the .cc file and renamed the method, but it's a good point this opportunity to reduce and simplify as well. Done. http://gerrit.cloudera.org:8080/#/c/20320/1/src/kudu/consensus/opid_util.h@72 PS1, Line 72: inline bool operator>=(const OpId& lhs, const OpId& rhs) { > Same as above comment but for <= operator, I guess it's only one line of co Done http://gerrit.cloudera.org:8080/#/c/20320/1/src/kudu/consensus/opid_util.cc File src/kudu/consensus/opid_util.cc: http://gerrit.cloudera.org:8080/#/c/20320/1/src/kudu/consensus/opid_util.cc@73 PS1, Line 73: 81 > +1 Ah, that's just the maximum number of characters that you might get if using positive int64_t numbers in decimal representation for the "$0.$1-$2.$3" pattern below, and then there is the opening '[' and closing ']' brackets: 4 * 19 + 3 + 2 = 81. I think it's worth adding extra 1 for the case if c_str() is called on the return string, so '\0' might be added depending on the implementation and the copy-on-write optimizations for std::string -- To view, visit http://gerrit.cloudera.org:8080/20320 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea1e7fba9d91ac9527d88721a867a856e8a858b3 Gerrit-Change-Number: 20320 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Tue, 08 Aug 2023 19:23:37 +0000 Gerrit-HasComments: Yes
