Todd Lipcon has posted comments on this change. Change subject: env_util: Implement CreateDirsRecursively() ......................................................................
Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: Line 202: for (auto path = paths.crbegin(); path != paths.crend(); ++path) { > I was trying to write it to abstract away the details of what a path is, an I'm skeptical of the value of abstraction in this case since it obscures what's going on and makes the code harder to follow. Even in the case that we eventually ported to Windows at some point, it would be a relatively simple change to use '\' instead of '/' here. If you really want to make the portability argument, I'd say introduce a constant 'kPathSeparator' in path_util.h which is set to '/' (analogous to python's os.path.sep). Or introduce a function SplitPath() in path_util. But repeatedly calling DirName like this is just more confusing IMO. -- To view, visit http://gerrit.cloudera.org:8080/5618 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
