Here's my first take on Path.cpp. It has a few holes still but it ought to be basically functional. A few observations: First, the Win32 version still has a lot of code in common with Unix. This code should be pulled out into a common file. Second, some of the functions leave me scratching me head. What purpose does create_file serve? Is there really a need to create empty files? (It also leaks an open file handle, BTW). I would expect set_file to alter only the file name portion of the path and leave the directory portion unchanged, and the opposite for set_directory. set_directory obliterates the file name portion of the path (OK, I guess) but set_file doesn't make any sense at all. -------------- next part -------------- A non-text attachment was scrubbed... Name: Path.diff Type: application/octet-stream Size: 10799 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20040918/681287f5/attachment.obj>
On Sat, 2004-09-18 at 12:01, Jeff Cohen wrote:> Here's my first take on Path.cpp. It has a few holes still but it ought > to be basically functional. > > A few observations: > > First, the Win32 version still has a lot of code in common with Unix. > This code should be pulled out into a common file.truly generic code (common to all platforms) should go in lib/System/Path.cpp. If you know of such things, feel free to send patches. FYI: I'll reject anything that has a / in a string literal. That's not portable to all systems. While it might work on 99.9% of unix and some modern Win32 machines, it won't work on VMS or OS/390.> > Second, some of the functions leave me scratching me head. What purpose > does create_file serve? Is there really a need to create empty files?Yes. Lock files are done this way. However, I'm not sure if LLVM uses them.> (It also leaks an open file handle, BTW).Oops. I'll take a look.> I would expect set_file to > alter only the file name portion of the path and leave the directory > portion unchanged, and the opposite for set_directory. set_directory > obliterates the file name portion of the path (OK, I guess) but set_file > doesn't make any sense at all.The Path class is intended to provide path construction, not path editing. I don't see any need for path editing in LLVM. So, "set_file" just sets the *entire* path to a single file name. I suppose it could prefix it with ./ on Unix since that's what it means (i.e. the directory part is the current directory. Set directory similarly sets the *entire* path to a single directory name. The various append_* and elide_* functions assist with path construction, in limited ways. set_file makes perfect sense ;> Reid. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: This is a digitally signed message part URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20040918/f0589913/attachment.sig>
Patch looks good so I committed it. I'd be interested in knowing what your test results are once you start testing with the Win32 port. Reid. On Sat, 2004-09-18 at 12:01, Jeff Cohen wrote:> Here's my first take on Path.cpp. It has a few holes still but it ought > to be basically functional. > > A few observations: > > First, the Win32 version still has a lot of code in common with Unix. > This code should be pulled out into a common file. > > Second, some of the functions leave me scratching me head. What purpose > does create_file serve? Is there really a need to create empty files? > (It also leaks an open file handle, BTW). I would expect set_file to > alter only the file name portion of the path and leave the directory > portion unchanged, and the opposite for set_directory. set_directory > obliterates the file name portion of the path (OK, I guess) but set_file > doesn't make any sense at all. > > ______________________________________________________________________ > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://mail.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: This is a digitally signed message part URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20040918/d6ee44b0/attachment.sig>
On Sat, 18 Sep 2004 12:30:41 -0700 Reid Spencer <reid at x10sys.com> wrote:> Patch looks good so I committed it. I'd be interested in knowing what > your test results are once you start testing with the Win32 port.I tested Signals.cpp and verified that the CTRL/C handler works, as does the stack trace. In fact, here's a sample: 77E73887 (0xE06D7363 0x00000001 0x00000003 0x0012FF28), RaiseException()+0080 bytes(s) 10226DB9 (0x0012FF44 0x0040DEFC 0x00020024 0x00647373), _CxxThrowException()+0057 bytes(s) 00401822 (0x0012FFC0 0x00409A2C 0x00000001 0x003250D0), XYZ::func()+0034 bytes(s), c:\projects\llvm\test.cpp, line 12 004017ED (0x00000001 0x003250D0 0x00322C68 0x00020024), main()+0013 bytes(s), c:\projects\llvm\test.cpp, line 19 00409A2C (0x00020024 0x7FFDF000 0x7FFDF000 0xF3893CF0), mainCRTStartup()+0300 bytes(s), f:\vs70builds\3077\vc\crtbld\crt\src\crtexe.c, line 398+0017 byte(s) 77E814C7 (0x00409900 0x00000000 0x78746341 0x00000020), GetCurrentDirectoryW()+0068 bytes(s) However, for some reason I haven't been able to determine, the lovely trace you see above comes out only on Windows XP. On Windows 2000, it can't get the symbol information for EXEs, though it does for DLLs. I know this code worked on 2000 in the past, so it must be something that broke with VC 7.1. Oh well. (The GetCurrentDirectoryW is bogus, but the top of the stack usually is). I did make some minor changes that I'll submit shortly.