- User Since
- Jan 26 2015, 9:26 AM (311 w, 5 d)
Wed, Jan 13
Thanks for the changes!
Mon, Jan 11
Fri, Jan 8
This looks good to me, in that I believe it will correctly parse typical native Windows path names. Only tests will tell, but I understand that there are many patches necessary to enable the tests.
LGTM, but please take the suggested edit to update a documentation URL.
Tue, Jan 5
Dec 17 2020
This looks correct and straightforward, so LGTM.
Dec 11 2020
Your call on using the CRT. I was under the mistaken impression that libcxx would implement both the standard C++ library as well as the CRT.
Dec 10 2020
LGTM iff this function never has to deal with paths prefixed with \\?\, as in \\?\C:\Windows or \\?\UNC\server\foo.
Dec 8 2020
Dec 7 2020
Oh, are there tests that need to be enabled on Windows?
I'm glad @gkistanova asked for this simplification. The more cautious approach added a lot of probably unnecessary noise.
Dec 4 2020
This looks like it'll work just fine, but I've added some inline questions for readability and robustness.
Dec 3 2020
Dec 2 2020
I'm out of my depth on 128-bit arithmetic and ABIs.
Dec 1 2020
These test look pretty thorough. I think these will be useful in finding portability problems.
I think this patch is a good idea, but I'm having second thoughts about whether this will solve the problem.
LGTM. (I must have slipped on the Action drop-down.)
I look forward to just switching everything to RemoveDirectory.
Nov 30 2020
I prefer the relatively localized disabling of the warning with the #pragmas as you've done as opposed to building the sanitizer builds with different settings.
Nov 20 2020
LGTM. I'll leave it to your discretion on the new_time range check.
Nov 19 2020
This looks good. I'm just confused by one point and have a small suggested edit.
I added two suggested edits for your consideration.
Nov 18 2020
LGTM as long as the run-time calculation of the default value for set_errno is legit.
I'm not a libcxx owner, so I'm not sure what other hoops you should jump through for them, but this looks good to me. Thanks for the improvements!
Nov 17 2020
Excellent. Somehow I missed that AreFileApisANSI was a Win32 function. I must have mistyped it in my searches the other day. This looks good.
Nov 16 2020
The Windows details look correct, though I have a couple questions on the code page conversions.
Nov 12 2020
I have a couple questions to consider inline. If those have already been thought through, then LTGM.
This is pretty close.
Oct 23 2020
This looks pretty good, both the patch and Pavel's insights. I don't see much to comment on that Pavel didn't already catch.
Oct 19 2020
It seems weird that we're implicitly adding defaultlib options without checking if the user specified nodefaultlib. But given that's already the case, I don't see a big problem with also adding oldnames.lib.
Oct 15 2020
I have no reason to suspect those patches in particular. I did a huge git
bisect, which turned up nothing. Older versions that used to work no
longer worked by the time the bisect worked that far back. That makes me
think it might be incomplete dependency checking: Something goes bad, and,
instead of rebuilding it, the bad result is re-used in each subsequent
Oct 14 2020
Oct 13 2020
If I had to guess, my money would be on a deadlock. To unblock, I'd
propose reverting this patch until we can figure it out.
Oct 12 2020
Oct 9 2020
LGTM. Thanks for extending this functionality to Windows!
Oct 8 2020
This looks pretty good.
Oct 7 2020
If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious.
It's interesting that I haven't encountered some of these errors. There are five _other_ lldb tests that do fail for me. I have a fix in the works for some of those.
Oct 6 2020
Some of this is nitpicky/opinionated, but the race condition is real. We need a reliable way to signal the watcher thread when it's time to exit. Options I see are:
Yes, I also (independently) discovered this problem. I probably have notes somewhere with sources for details about the incompatibility. I believe I also brought it up on the lldb-dev list.
Oct 5 2020
I don't know whether Python and our config tools use the terms consistent, but Win32 is the name of the Windows API used on both 32- and 64-bit processors, Win16 was the API used on 16-bit machines, and Win64 is a made-up term that confuses the issues.
Oct 2 2020
I'm sorry. I haven't had to time to review the entire change yet, but I thought I'd share some early feedback now and take more of a look on Monday.
Sep 29 2020
Sep 25 2020
A big portability problem with the virtual file system (VFS) is that we now have paths that can legitimately be in a hybrid Windows/Posix style. Be aware that some of the "guess-the-style" instances were modified to account for this.
Sep 21 2020
Sep 15 2020
I never realized rc.exe would accept having too few values for a FILEVERSION or PRODUCTVERSION. The documentation implies that you must provide four components--it is the _fixed_ portion of the VERSIONINFO after all. Not that it matters; this looks harmless.
Sep 2 2020
Aug 31 2020
I like it.
Aug 28 2020
Is there a more specific term than "traditional linker"?
Aug 27 2020
The change looks fine.
Aug 26 2020
Oops, I forgot to "Accept Revision."
Not my area of expertise, but this looks good to me. Before submitting, please make sure you update the commit description since you didn't end up adding any fields to SectionChunk.
I know it's not part of this patch, but Phabricator is showing a clang-tidy lint error on the inclusion of <io.h>. While Windows does have one of these, it's only a compatibility hack, and it doesn't seem to be needed. Can you yank that #include <io.h> line before submitting?
Aug 14 2020
Aug 13 2020
Landed on Tuesday. I must have messed up the Differential Revision: line.
Aug 12 2020
Aug 11 2020
If you make the second loop consistent, I'll approve, despite my concerns about the related problems.
Aug 10 2020
I'm OK with this.
Aug 7 2020
OK, I see. I was looking at the wrong field.
Huh, I thought aggregate initialization was effectively zero-initialization for the remaining fields, so I'm surprised this changes anything. I would have expected the original line, which is idiomatic in Win32 code, to have zero-initialized all of the fields.
Added test to check locating the PDB either in the original build directory or adjacent to the executable.
Aug 4 2020
OK, makes sense. I was hoping the processing could happen as you build up the concatenated string rather than at the end. But now I see the design doesn't really allow for that.
I'll defer to @thakis, but I have a couple suggestions inline.
Aug 3 2020
Well then, since Reid is out, this is good to go! :-)
I have a couple responses, but nothing worth blocking this path for. LGTM, and thanks for doing this!
Jul 30 2020
Thanks. Working on a test.
Jul 29 2020
Jul 28 2020
Fixed typos in comments.