You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/01/05 21:15:25 UTC
[kudu-CR] path util: Add IsRelativePath() helper function
Hello Dinesh Bhat, Lars Volker, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5617
to review the following change.
Change subject: path_util: Add IsRelativePath() helper function
......................................................................
path_util: Add IsRelativePath() helper function
Change-Id: Ieae26e50fb299fc9227cbc2d212bce6db0c11571
---
M src/kudu/util/path_util-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/5617/1
--
To view, visit http://gerrit.cloudera.org:8080/5617
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieae26e50fb299fc9227cbc2d212bce6db0c11571
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[kudu-CR] path util: Add IsRelativePath() helper function
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: path_util: Add IsRelativePath() 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;
> warning: redundant boolean literal in conditional return statement [readabi
is an empty path really valid at all? Given that I wouldn't expect valid paths to be ever specified as empty, this function devolves to just checking that the string starts with '/', and then I'm not sure there's much value in it vs just inlining the condition wherever you want to use this.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] path util: Add IsRelativePathOrEmpty() helper function
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy 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;
> btw my concern isn't anything about ability to inline - more that when I'm
> I think we've already lost the battle
The idea was simply not to add to the portability problem. But you guys apparently think that this is net-negative so I will back it out.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] path util: Add IsRelativePathOrEmpty() helper function
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change.
Change subject: path_util: Add IsRelativePathOrEmpty() helper function
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/5617
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ieae26e50fb299fc9227cbc2d212bce6db0c11571
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] path util: Add IsRelativePathOrEmpty() helper function
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] path util: Add IsRelativePathOrEmpty() helper function
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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;
> I think we've already lost the "encapsulate path separators to ease porting
btw my concern isn't anything about ability to inline - more that when I'm reading some code that uses this function, I think to my self "oh, this function must be doing something interesting, I better go page it in" whereas if I just saw foo.startswith("/") I'd immediately get it without having to go look at the definition.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] path util: Add IsRelativePath() helper function
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: path_util: Add IsRelativePath() 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;
> I agreed with Todd.
My reasoning behind empty being relative is that if you were to concatenate a bunch of paths, and one was a directory that was empty, then we would consider that relative, and basically equivalent to ".".
Regarding why have this at all, I thought if we ever wanted to point to non-POSIX then it's one less thing scattered everywhere. So might as well add a helper that says what we mean. If the concern is ability to inline, I suppose I could make it a header-only static function.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] path util: Add IsRelativePath() helper function
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: path_util: Add IsRelativePath() 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;
> is an empty path really valid at all? Given that I wouldn't expect valid pa
I agreed with Todd.
Further, I think if one argues for the validity of an empty path, that path is actually absolute, not relative. That is, '' is equivalent to '/', not '.'.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] path util: Add IsRelativePathOrEmpty() helper function
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5617
to look at the new patch set (#2).
Change subject: path_util: Add IsRelativePathOrEmpty() helper function
......................................................................
path_util: Add IsRelativePathOrEmpty() helper function
Change-Id: Ieae26e50fb299fc9227cbc2d212bce6db0c11571
---
M src/kudu/util/path_util-test.cc
M src/kudu/util/path_util.h
2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/5617/2
--
To view, visit http://gerrit.cloudera.org:8080/5617
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieae26e50fb299fc9227cbc2d212bce6db0c11571
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>