Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14569 )
Change subject: util: add ToLowerCase string case methods ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/14569/1/src/kudu/util/string_case.h File src/kudu/util/string_case.h: http://gerrit.cloudera.org:8080/#/c/14569/1/src/kudu/util/string_case.h@46 PS1, Line 46: Lower-case > nit: while you are at it, could you add that this is about ASCII upper/lowe Done http://gerrit.cloudera.org:8080/#/c/14569/1/src/kudu/util/string_case.h@49 PS1, Line 49: *out > nit: mind fixing the spacing here and above? Done http://gerrit.cloudera.org:8080/#/c/14569/1/src/kudu/util/string_case.cc File src/kudu/util/string_case.cc: http://gerrit.cloudera.org:8080/#/c/14569/1/src/kudu/util/string_case.cc@65 PS1, Line 65: *out > nit: same here Done http://gerrit.cloudera.org:8080/#/c/14569/1/src/kudu/util/string_case.cc@70 PS1, Line 70: ascii_tolower > Is it OK to ignore locale and just use ASCII conversion instead? Good question. >From looking at the usage of ToUpperCase and ToLowerCase, we mostly call it on >our own static strings (which are definitely ASCII-compatible), but we do >sometimes call it on a user-provided string. AFAICT, the goal is always to >compare the result against a statically defined set of strings (i.e. >encodings, compressions, type names, etc.), meaning the user should have >provided an ASCII-compatible string to begin with. So I don't think it's dangerous to ignore the locale here. If it's called on a non-ASCII string, GIGO. -- To view, visit http://gerrit.cloudera.org:8080/14569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d9ddb28175a328dbd23549bee527c33f7a519b8 Gerrit-Change-Number: 14569 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 30 Oct 2019 00:28:21 +0000 Gerrit-HasComments: Yes
