[PATCH] Fixes a bug when iterating on paths

Rafael Espíndola rafael.espindola at gmail.com
Fri Aug 9 14:35:07 PDT 2013


ok. Do you have commit access?

On 9 August 2013 17:04, Tareq A. Siraj <tareq.a.siraj at intel.com> wrote:
>
>
> ================
> 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