clayborg added inline comments.
================
Comment at: source/Utility/FileSpec.cpp:406
+ m_directory.Clear();
+ }
}
----------------
labath wrote:
> clayborg wrote:
> > > they resolve to the same file in a virtual filesystem, where all
> > > referenced path components exist and are directories
> >
> > Normalizing happens after resolving and if we don't resolve a path, we have
> > no way to know what is a directory and what isn't. We will be setting
> > breakpoints for remote targets quite a bit in LLDB and ww can't assume or
> > stat anything in a path. So I would say FileSpec's are equivalent if the
> > relative paths match all components.
> >
> > > Now the (c) part would imply that foo/.. should normalize to ./. and not
> > > ., which is a bit odd, but it is consistent with our stated intention of
> > > preserving directory information. If we do not want to have ./. here,
> > > then we need to come up with a different definition of what it means to
> > > be "normalized".
> >
> > I guess we could make the m_directory contain "." and m_filename contain
> > nothing for the "./." case. It doesn 't make sense to have "." in both
> > places.
> >> they resolve to the same file in a virtual filesystem, where all
> >> referenced path components exist and are directories
>
> > Normalizing happens after resolving and if we don't resolve a path, we have
> > no way to know what is a directory and what isn't. We will be setting
> > breakpoints for remote targets quite a bit in LLDB and ww can't assume or
> > stat anything in a path.
>
> Yes, I am aware of that. I am not saying this is how we should actually
> implement the normalization algorithm. I am trying define what a
> "normalization" is in the first place, so that we can then judge whether a
> particular normalization algorithm is good or not. I think defining
> normalization in terms of an actual filesystem makes sense, since at the end
> of the day, our algorithm should somehow approximate what happens in real
> file systems. I am not saying the algorithm should be doing any stats, but
> for the verification (either in our heads or in the tests) we can use
> certainly use stats or actual file systems.
>
> > So I would say FileSpec's are equivalent if the relative paths match all
> > components.
>
> This is too vague to be useful. I have no idea how I would apply this
> definition to determine if e.g. "/foo/../bar.txt" and "./bar.txt" are
> equivalent. And you didn't say anything about how to derive the normal form
> for a `FileSpec`.
>
> >> Now the (c) part would imply that foo/.. should normalize to ./. and not
> >> ., which is a bit odd, but it is consistent with our stated intention of
> >> preserving directory information. If we do not want to have ./. here, then
> >> we need to come up with a different definition of what it means to be
> >> "normalized".
>
> > I guess we could make the m_directory contain "." and m_filename contain
> > nothing for the "./." case. It doesn 't make sense to have "." in both
> > places.
>
> I don't think that is very useful, as then this would be the only special
> case where normalization would produce a FileSpec without a filename
> component.
>> So I would say FileSpec's are equivalent if the relative paths match all
>> components.
> This is too vague to be useful. I have no idea how I would apply this
> definition to determine if e.g. "/foo/../bar.txt" and "./bar.txt" are
> equivalent. And you didn't say anything about how to derive the normal form
> for a FileSpec.
"/foo/../bar.txt" would be normalized to "/bar.txt" and "./bar.txt" will, after
switching to llvm's remove_dots, be normalized to "bar.txt". So those could be
though of as equivalent since one is only a basename and would only need to
match the filename. If you have "/foo/bar/baz.txt", it could be equivalent to
"bar/baz.txt" by making sure the filename's match, and if either or both path
is relative, then matching as many directories as are specified.
>> I guess we could make the m_directory contain "." and m_filename contain
>> nothing for the "./." case. It doesn 't make sense to have "." in both
>> places.
> I don't think that is very useful, as then this would be the only special
> case where normalization would produce a FileSpec without a filename
> component.
Ok, then leave it as is with "." in filename, not directory. We don't need it
in both places IMHO
================
Comment at: source/Utility/FileSpec.cpp:857
+ !IsPathSeparator(components[i].back(), syntax) &&
+ (result.empty() || !IsPathSeparator(result.back(), syntax)))
result += GetPreferredPathSeparator(syntax);
----------------
labath wrote:
> clayborg wrote:
> > labath wrote:
> > > Why is this necessary? `result` can't be empty because we have just
> > > appended something to it (and we've checked that `components[i]` is
> > > non-empty).
> > Because if you don't check this joining {"/', "foo.txt"} will result in
> > "//foo.txt" which is wrong,.
> Yes, but didn't the original condition guard against that already?
>
> We know that `components[i]` is non-empty, and we have just appended it to
> `result` two lines above. So, unless I am missing something, `result.back()`
> should be the same as `components[i].back()` and this additional check does
> not buy us anything.
I did not. During testing I found this case and "/" + "main.c" was producing
"//main.c"
https://reviews.llvm.org/D45977
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
