- User Since
- Jan 26 2015, 9:26 AM (426 w, 5 d)
Sep 21 2021
Sep 16 2021
I'm trying to understand why I've never seen the problem that this patch is trying to address.
Sep 13 2021
Thanks for the fixes. LGTM.
Sep 10 2021
Minor suggestions, but otherwise LGTM. I assume you've coordinated with @aganea to ensure you aren't duplicating effort.
Aug 31 2021
I'm mildly surprised the make_absolute call doesn't remove unnecessary dotted paths. I guess that's related to the weird schism between sys::fs and sys::path.
Aug 27 2021
This is a larger change than I anticipated. I was surprised by the need to have the pointer be thread local (and then ensure that the thread local pointers for new threads are set up, too). If I'm understanding correctly, the idea is that a single program could have n threads doing n links at the same time. Is that right? I'm not arguing against that. I'm just trying to make sure I understand why I was surprised.
Aug 24 2021
Aug 23 2021
I also thank you for working to improve this documentation. I mostly agree the other reviewer. I've only added a couple inline comments to raise questions. I wouldn't hold this back for either of those.
Aug 20 2021
Aug 12 2021
Aug 11 2021
Aug 10 2021
Thanks for making the change more local.
Aug 6 2021
Aug 4 2021
I was a little thrown by llvm-windres versus llvm-rc. I hadn't heard of llvm-windres before, and my web search came up with zero hits (literally!). But now I understand.
Jul 23 2021
I see. I didn't realize llvm-rc used an actual separate preprocessor step. (RC has a crude one built-in that doesn't quite match any of the standards.)
Cool. I'd never seen anyone use strings that don't look like a C or C++ identifier as a resource identifier, but it should work given that FindResource just uses the string as an opaque key.
Jul 8 2021
Two small requests, then LGTM.
Jul 2 2021
tl;dr: I understand the desire to avoid even tiny synchronization costs when trying to read a high-precision timer, but I'm uncomfortable with the data race. I suggest initializing the static function pointer under the locking provided by the compiler.
Jun 25 2021
Jun 22 2021
The idea looks fine. Just a couple inline questions.
Jun 17 2021
Jun 11 2021
This patch was reverted a while back because a couple DirectoryWatcher tests routinely timed out on build bots even though they work when run in isolation.
Thanks for the changes. This LGTM now.
Jun 9 2021
Nice catch Reid.
Jun 7 2021
Jun 3 2021
Jun 1 2021
I'm fine focusing on this part to get the bug fixed. We can consider looking at accepting upper and lower case letters in the future.
May 27 2021
Yeah, this is good. My remaining comments are all speculations about how to improve this further in the future, but they aren't directly applicable to the goal here.
May 25 2021
This sounds like an appropriate place to apply Postel's Law: Be conservative in what you do, be liberal in what you accept from others. I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.
May 24 2021
May 21 2021
Cool! This is getting much simpler. My remaining comments are mostly musings and you can tell me to buzz off.
May 19 2021
Sorry, forgot to Request Changes.
At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done.
May 18 2021
I'll have to dig into my archived email tomorrow.
May 13 2021
When this gets further along, maybe we can get LLDB to use it instead of EditLine.
May 12 2021
May 11 2021
LGTM. Dead code should go. If somebody wants to own a Windows port of EditLine, they can re-instate it and put some tests on it.
It looks like this EditLine port was added before I joined this project. @zturner may have worked on it, but I don't know/remember.
May 10 2021
May 5 2021
Apr 29 2021
Apr 28 2021
Apr 27 2021
A drive-by look.
Apr 26 2021
There's a lot going on here, but I don't see anything wrong. Thanks for the completeness of the tests and the comments, as that helps a lot in understanding what's going on here.
Apr 21 2021
Wow. Excellent. Thank you!
Apr 20 2021
I'll merge it for you.
Apr 19 2021
Ah, I get it. Thanks for the clarification.
I'm not quite clear on the motivation. It's hard for me to imagine the resource compiler being useful if you cannot use the C preprocessor. I guess for simple tests you don't need it, is that worth adding an option?
Apr 16 2021
Perhaps somebody confused <utility> std::move with <algorithm> std::move.
Apr 14 2021
Personally, I'm not a fan of boolean function parameters because of the inline comments necessary to make the call site understandable. But it appears to be consistent with LLVM Coding Standards and other APIs, so this looks right to me.
Apr 12 2021
Sorry, I think I've lost track of some context while I was on vacation. I don't understand why several of the tests are now unsupported on Windows. Some of those seem like important tests.
Apr 1 2021
Mar 30 2021
The previous discussions (that I participated in) were centered around the redirecting virtual filesystem, which creates paths in hybrid style and there is no "correct" way to make those native without taking breaking changes and making it less useful for writing platform-agnostic tests. But it's not clear that's relevant here.
I like the composability enabled by making OF_Text and OF_CRLF independent. I wonder, though, if there's a chokepoint where we could/should assert if the caller uses OF_CRLF without OF_Text, which would be suspicious.
Mar 29 2021
Adding everything in LIB to PATH is not a normal thing to do in Windows, so I would support this patch to remove that hack. If we knew what the original problem was, we could probably find a better solution.
Mar 25 2021
Just some drive-by comments.
When changing an IO stream's mode from binary to text breaks only Windows, it's likely due to the fact that Windows uses CR+LF as the newline indicator. Not only are the CRs sometimes unexpected, they can also throw off code that tries to seek to a computed file position.
Mar 24 2021
This looks even better.
Mar 23 2021
I like the solution. Has anyone actually tried it on Windows 7? I ask because some of the observations noted in second hand descriptions of the problem didn't fully add up. I think we understand the root problem, but I'm not 100% certain.
Mar 17 2021
Minidumps should have the registers in the processor context. It seems LLDB knows about TCR_ELn for n > 0. Maybe TCR_EL0 is special?
Mar 16 2021
This workaround seems consistent with other overrides of FixCodeAddress, my only concern is about the assumption of the number of bits that need to be preserved/stripped.
Mar 5 2021
Mar 4 2021
From a Windows point of view, this LGTM.
Feb 26 2021
Looks good enough to me. (But I'm also not a libc++ owner.)
Feb 25 2021
Feb 24 2021
The option is used in the wild, but not as widely as I first believed. We've already fixed a couple projects that were using it. I think the Release Note is the right solution. Thanks for doing that. I withdraw my suggestion to allow and ignore.
Feb 19 2021
I'm in no position to evaluate the functionality of lldb on Windows. I've spent the last few months trying to understand why 900+ tests spontaneously started failing on Windows.
Feb 18 2021
Fair enough. I'm continuing the whack-a-mole game for the Chromium third-parties.
The removal of -Wreturn-std-move-in-c++11 breaks a few projects. These projects are specifying the option, which then triggers an unknown-option warning and thus -Wx halts the build.
Feb 16 2021
I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.
Feb 12 2021
Jan 29 2021
The changes to provide realpath on Windows looks right. Since it's relying on a system call, it would be difficult to create tests for the \\?\ and UNC cases. I'm assuming the more mainstream cases will be tested by existing tests. So LGTM.
Jan 22 2021
BTW, I hope I didn't come across as overly negative in my previous
response. I'd love to see the situation improved. I just don't know what
that would look like.
Jan 21 2021
I didn't design VFS. I just fixed a bunch of portability bugs that
prevented us from running most of the VFS tests on Windows. If I were
designing it, I (hope) I wouldn't have done it this way. But the horse is
already out of the barn.
Jan 13 2021
Thanks for the changes!
Jan 11 2021
Jan 8 2021
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.
Jan 5 2021
Dec 17 2020
This looks correct and straightforward, so LGTM.
Dec 11 2020