Adar Dembo has posted comments on this change. Change subject: path_util: Add IsRelativePathOrEmpty() helper function ......................................................................
Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/5617/1/src/kudu/util/path_util.cc File src/kudu/util/path_util.cc: Line 40: if (path.empty() || path[0] != '/') return true; > My reasoning behind empty being relative is that if you were to concatenate I think we've already lost the "encapsulate path separators to ease porting to Windows" battle. We've leaked UNIX filesystem semantics (i.e. forward slash as path separator, absolute paths begin with a single path separator, etc.) all over the place. Some examples: util/test_util.cc, util/trace.cc, fs/fs_manager.cc, gutil/strings/escaping.cc. So I don't see why we should start holding the line in this patch. If you want to attack the problem structurally and address all of the violators, by all means. But what good does a single well-behaved function in path_util.h do with so many offenders elsewhere? -- To view, visit http://gerrit.cloudera.org:8080/5617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieae26e50fb299fc9227cbc2d212bce6db0c11571 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
