[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