[Lldb-commits] [PATCH] [2/2] Fix Path support on Windows
gclayton at apple.com
Wed Jul 30 18:50:17 PDT 2014
Looks ok, a few questions:
Why does the user ever want to specify the PathSyntax? Shouldn't it just be something that can be queried and be set automagically?
What if I did:
FileSpec f("C:\Users\me\foo.txt", false, ePathSyntaxPosix);
If I set it incorrectly, seems like it might "do the wrong thing" in some calls. It might be nice to use a C++11 typed bitfield to store the PathSyntax to keep the size of the FileSpec class down to as small as possible (uint8_t).
All the other functionality seems fine.
A few things to keep in mind:
All debug info is parsed and stored into FileSpec objects. They must match exactly what the user types. What if the user types in:
(lldb) breakpoint set --file C:/Users/me/foo.txt
Note the wrong slashes. Are we alway normalizing a path to be native so that this kind of compare will always work even if the user types it in wrong?
Seems like we shouldn't allow the user to specify the PathSyntax or we can run into problems.
Also seems like we should check if a string needs to be normalized before calling Normalize and allocating memory and copying and replacing things which can be expensive (think parsing thousands of line table files).
So I would vote to:
1 - fix it so the user never specifies PathSyntax, but can query it if desired
2 - I would rather not normalize the path on every FileSpec::SetFile(...) if we can avoid it as it will be expensive
3 - If we do need to normalize it every time, either have Normalize return false and not touch the llvm::SmallString<64> argument (no mallocs) if it doesn't need to be normalized to avoid expensive allocations if not required.
> On Jul 30, 2014, at 4:49 PM, Zachary Turner <zturner at google.com> wrote:
>>> ! In D4675#11, @deepak2427 wrote:
>> Thanks for fixing this. I haven't had a chance to test this patch with our cross-platform issues yet, have to look at some other issues first.
>> Just a couple of doubts I had.
>> - Are the paths always going to default as ePathSyntaxHostNative and then converted based on the Host?
>> - In 733 llvm::SmallString<64> denormalized; , is the length of 64 enough?
>> - I'm not that familiar with LLVM data structures, however would it be better to keep llvm::SmallString itself for the Normalize/Denormalize functions to keep it consistent?
> Paths default to SyntaxHostNative because that makes the semantics post-patch equivalent to pre-patch. Normalized form is basically unix form, so if the syntax ends up as ePathSyntaxPosix (either because it was explicitly specified or because host was specified and your host is posix), then normalization and de-normalization are both no-ops.
> SmallString<64> just means that it keeps 64 bytes on the stack, and if it grows longer than that, it will fallback to help allocation. It still supports arbitrarily long strings, but paths tend to be small, so that's why a small number was chosen.
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
More information about the lldb-commits