[PATCH] Fixes a bug when iterating on paths
Tareq A. Siraj
tareq.a.siraj at intel.com
Fri Aug 9 14:04:30 PDT 2013
================
Comment at: lib/Support/Path.cpp:80
@@ -79,3 +79,3 @@
// * {file,directory}name
- size_t end = path.find_first_of(separators, 2);
+ size_t end = path.find_first_of(separators, 0);
return path.substr(0, end);
----------------
Rafael Ávila de Espíndola wrote:
> 0 is the default, please drop it
Fixed.
================
Comment at: unittests/Support/Path.cpp:150
@@ +149,3 @@
+
+ StringRef(Path).split(ExpectedPathComponents, "/", -1, false);
+
----------------
Rafael Ávila de Espíndola wrote:
> Nit: why do you need to pass -1 and false?
>
Actually, in this case, we don't need it. I'll remove the extra params.
================
Comment at: unittests/Support/Path.cpp:173
@@ +172,3 @@
+ // The root path will also be a component when iterating
+ ExpectedPathComponents.insert(ExpectedPathComponents.begin(), "/");
+
----------------
Rafael Ávila de Espíndola wrote:
> Might be cleaner to not pass -1 and false to split and then do
>
> ExpectedPathComponent[0] = "/";
>
I thought the intention would be more readable this way. The `false` argument here makes sure there are no empty strings in the vector. In this case since the path starts with a separator, the first element will be empty without the `false` to split and `ExpectedPathComponent[0] = "/";` would work. Otherwise we need to insert instead of replacing the element.
That being said, I'm ok with the solution you provided too.
================
Comment at: unittests/Support/Path.cpp:198
@@ +197,3 @@
+ // when iterating.
+ ExpectedPathComponents.insert(ExpectedPathComponents.begin()+1, "\\");
+
----------------
Rafael Ávila de Espíndola wrote:
> Same as above.
Here, I think it is necessary to `insert` instead of replace. I can remove the extra arguments to `split` since in this case there won't be any empty strings but we do need to `insert` the separator in the vector to avoid replacing existing non-empty components.
http://llvm-reviews.chandlerc.com/D1277
BRANCH
svn
ARCANIST PROJECT
llvm
More information about the llvm-commits
mailing list