- User Since
- Jan 26 2015, 9:26 AM (246 w, 2 d)
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.
Mon, Oct 14
Thu, Oct 10
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.
Wed, Oct 9
This matches the behavior of cl.
Tue, Oct 8
LGTM after one question.
Given Pavel's comment, this LGTM.
Is this still an active review or has this been abandoned?
Wed, Oct 2
Tue, Oct 1
Why an environment variable rather than a command line option?
Mon, Sep 30
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.
Is this necessary? I see
Aug 16 2019
Aug 14 2019
It turns out these tests all seem to exist, so I guess Zachary submitted them as part of other CLs.
Aug 9 2019
Just closing the loop. Regarding the "Python memory allocator called without holding the GIL" bugs: this is not a "just me" problem. It's for anyone who enables debug mode. This appears to be a test infrastructure problem, but it might be harmless. If I run in "release" mode, I get the same behavior as the Windows build bot.
It looks like the code changes landed (probably as part of another patch) but not the tests. I'll look and see if the tests should to be revived.
Aug 8 2019
This looks fine, and thanks for the tests for a one-line fix.
Aug 5 2019
Yes, zturner isn't as active here as he was before.
Aug 1 2019
Jul 31 2019
Jul 30 2019
Jul 26 2019
Looks good. Thanks for doing these updates!
Jul 22 2019
Yes, I can submit it for you, probably in the next hour or two. Thanks for the patch!
Jul 18 2019
Thanks for the change. I was thinking Expected based on the name of the function: GetOrCreateDeclForUid. I was thinking that it would be odd to have a UID if you don't have debug info. Anyway, Optional is fine. Thanks again.
Jul 17 2019
An aside ...
This change looks fine to me, but I wish there was a reliable way to test it. Is it a true race condition (e.g., because the unpredictability of thread scheduling), or is there a way to write a test that would always fail without this fix?
Oh, and thanks for the background and context on the motivation for this change!
LGTM, but please reconsider whether the return type of GetOrCreateDeclForUid should be changed in this patch rather than in a future one.
Jul 16 2019
OK, the only way I was able to make this work was to remove all traces of Python 2.x from my machine. As long as the older Python existed on my machine CMake would find that one, regardless of which one was in my PATH or indicated by PYTHON_HOME_DIR. Of course, it required killing the cmake cache a couple times, too.
Actually, right now I'm trying to figure out where the interpreter is found because cmake is finding different, incompatible versions of the interpreter (2.7) and the libs (3.6).
Was there a proposal or discussion (e.g., a thread on lldb-dev) about making PdbAstBuilder abstract in order to handle other languages? It sounds like a good idea, but I'd like to catch up on any context I may have missed.
Jul 9 2019
Be aware that Swig 3.0.12 has a compatibility problem with Python 3.7 that causes hundreds of LLDB tests to fail an assertion (at least on Windows).
Jun 25 2019
The LGTM, but I wasn't nominated as a reviewer, and I was mostly looking at it from the point of Windows compatibility.
Jun 24 2019
zturner is not a regular here anymore. I think he pops in from time-to-time, but I wouldn't depend on just him for a review.
Jun 18 2019
Jun 17 2019
Jun 14 2019
I'm OK with moving common stuff out for now, and I like the separation of ProcessWindows and ProcessDebugger.
Jun 13 2019
It's been a while for me, so I'm not super-familiar with the code being changed, but I'm okay with factoring out common code. I agree with labath's open points and will try to look at it in more detail tomorrow.
Jun 11 2019
Sorry for the stupid question, but ...