- User Since
- Jan 26 2015, 9:26 AM (260 w, 6 d)
Fri, Jan 24
cc: +thakis, who expressed interest in seeing a fix for the text exempted on Windows.
Thu, Jan 2
It would surprise me if the different test config is cause here. The path is assembled entirely in the code, so it should be consistent. If you had a systemic problem introduced by test config, I'd expect more of the VFS tests to fail.
Mon, Dec 30
I'm still not seeing how the backslashes got in there for you. They don't happen for me. It looks like these paths come (almost) directly from the temp .yaml files created by the sed commands at the beginning of the test, so--for these particular tests--I'd expect forward slashes regardless of platform.
I've made a similar change in several other tests, but this test works for me without this change. I'm trying to understand why, but I don't see any harm in landing this.
Dec 20 2019
Just a nit on the patch description: This change replaces the CRT allocator (i.e., malloc and free). The "default Windows heap allocator" could be misunderstood to mean replacing the OS-level heap manager (i.e., https://docs.microsoft.com/en-us/windows/win32/memory/heap-functions).
Dec 19 2019
This landed in 3b69f0c5550a229dd6d39e361182cdd7cecc36a4, but there was a typo in the patch description so the tools didn't automatically close this.
Dec 18 2019
Dec 9 2019
Dec 5 2019
Hold off on this one. It needs an explicit test for canonicalization.
Dec 3 2019
A (hopefully) more cogent response than my last round. I'm still hoping to hear from VFS owners.
Dec 2 2019
I'm following along, but I don't think I have enough domain knowledge to qualify as a reviewer.
Nov 27 2019
Nov 26 2019
Is .eh_frame the only one that matters? Should this be more general and compare const_sect_name to the full name and the truncated name for any known section names?
That's fair. LGTM.
I'm good with the change, but have a couple small requests. I hope to hear from others, too, as this area is outside my wheelhouse.
Nov 25 2019
Nov 21 2019
Nov 20 2019
Reverted unintended change caught by reviewer.
Nov 19 2019
Nov 18 2019
labath has added great context here, and I generally agree with clayborg's ideal of optimal behavior. That said, if memory serves, getting that behavior on Windows can be quite challenging, so I'm not sure if it's worth the effort.
Nov 15 2019
Nov 14 2019
It's too bad that pattern is repeated three times, including the explanatory comment.
Nov 13 2019
Corrected comment about default for case sensitivity.
Thanks for the review.
Modified comment per feedback.
Friendly ping for any VFS experts to comment.
Nov 12 2019
This looks like the right approach, but I have a concern or two about the ramifications as noted inline. Let me know if my concerns are unwarranted.
Nov 7 2019
I create a new review thread for my improved patch shortly. These changes were too wide-ranging.
Nov 4 2019
Oct 30 2019
Thanks for identifying this race condition.
Oct 25 2019
FYI: This broke the build for me.
Somehow Phabricator failed to notify me that you'd already left comments. I even searched my inbox to see if I'd just missed the notification. Nope. Nothing. I came here today to ping you for the review and discovered you'd already done your part.
Oct 24 2019
Oct 21 2019
I'm OK with this. I'm just wondering whether it would be a good idea to make it clear that these header sections are "not considered to be a section in the strictest sense." Is the distinction going to be important to future code readers? Do we already have "sections" that aren't truly "sections"?
Oct 15 2019
I know there are some complexities with configuring msvc/clang-cl, where one needs to fetch registry keys and whatnot in order to get the right build environment.
Oct 14 2019
Oct 10 2019
On Windows, it _does_ rewrite argv, but it looks like it tries to not change whether it was relative/absolute, so I think this is fine.
Cool. I didn't know about InitLLVM. That makes things much cleaner.
Oct 9 2019
This matches the behavior of cl.
Oct 8 2019
LGTM after one question.
Given Pavel's comment, this LGTM.
Is this still an active review or has this been abandoned?
Oct 2 2019
Oct 1 2019
Why an environment variable rather than a command line option?
Sep 30 2019
I'm just back from vacation. I agree with Pavel that we need to hear more from Jason at this point. I'm still very interested in helping this land in some form.
Sep 13 2019
Sep 12 2019
I have a couple more questions and some renaming requests.
My concerns are satisfied.
Sep 11 2019
I've been trying to figure out how to implement the same functionality you've got here, so I'm very interested in helping you land this in some form.
Sep 5 2019
Thanks for the changes! I think this looks good now.
I don't have any specific code comments, but I do have a couple general questions and points to consider.
Yes, I can commit it for you soon.
Thanks for factoring out the duplication.
Sep 4 2019
The \\?\ prefix tells the Windows API layer not to parse the path strings and just pass it along to the file system. Ramifications:
Disregard my last comment. Phabricator wasn't showing me that latest, nor that the patch had already been submitted, which seems to be happening with increasing frequency.
LGTM. Please consider adding a comment to the assert that summarizes what you explained in the thread.
Sep 3 2019
This is looking pretty good.
This feels very familiar. I think I've reviewed a similar change back when we first implemented minidumps.
I'm open to this if we can reduce the code duplication.
Aug 30 2019
I'll look at this in detail soon, but a few caveats:
Aug 28 2019
Somebody else already did this while I was waiting for review.
I support anything that reduces the code path differences between user-entered commands and their SBAPI counterparts. Thanks for doing this!
Aug 27 2019
When I added my comments, Phabricator showed this patch as still open. Now it looks like it landed four hours before that. :-(
Aug 26 2019
I have a couple inline questions. After that, it looks fine.
Aug 23 2019
Aug 22 2019
A couple inline comments. I think this is looking pretty good.