Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12657 )
Change subject: KUDU-2721: support ranges in CPU lists ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/12657/8/src/kudu/gutil/sysinfo.cc File src/kudu/gutil/sysinfo.cc: http://gerrit.cloudera.org:8080/#/c/12657/8/src/kudu/gutil/sysinfo.cc@177 PS8, Line 177: int ParseMaxCpuIndex(const char* str) { > Should early out if 'str' is nullptr, or DCHECK on that. Done http://gerrit.cloudera.org:8080/#/c/12657/8/src/kudu/gutil/sysinfo.cc@202 PS8, Line 202: dash != nullptr > Isn't this implied by the next condition? That is, if dash == range_start, yes that's true. http://gerrit.cloudera.org:8080/#/c/12657/8/src/kudu/gutil/sysinfo.cc@202 PS8, Line 202: num_start == pos > It's not really clear under what circumstances num_start == pos. Is it for Yeah, "" or "5-" would be examples. Added comment. http://gerrit.cloudera.org:8080/#/c/12657/8/src/kudu/gutil/sysinfo.cc@205 PS8, Line 205: long start_idx = strtol(range_start, nullptr, 10); > Why strtol and not strtoul here? Wouldn't a negative number be interpreted That's true, I can avoid the < 0 checks below if I do that. -- To view, visit http://gerrit.cloudera.org:8080/12657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97311bfbcf70bea069e941b6e7f4f015fb781b3f Gerrit-Change-Number: 12657 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 05 Mar 2019 22:01:05 +0000 Gerrit-HasComments: Yes
