- User Since
- Jan 26 2015, 9:26 AM (238 w, 4 d)
A couple inline comments. I think this is looking pretty good.
Is this necessary? I see
Fri, Aug 16
Wed, Aug 14
It turns out these tests all seem to exist, so I guess Zachary submitted them as part of other CLs.
Fri, Aug 9
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.
Thu, Aug 8
This looks fine, and thanks for the tests for a one-line fix.
Mon, Aug 5
Yes, zturner isn't as active here as he was before.
Thu, Aug 1
Wed, Jul 31
Tue, Jul 30
Fri, Jul 26
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 ...
Jun 10 2019
Jun 7 2019
Changed the approach in the first fix to use explicit escaping after Joel Denny's comment got me to re-think it.
Jun 4 2019
May 31 2019
May 22 2019
May 9 2019
LGTM once you double-check the return value in the error case at the end of SymbolFileBreakpad::ParseUnwindRow.
May 8 2019
Apr 30 2019
This looks fine to me, and the rationale sounds right. I'll be curious to see if any other reviewers see a potential problem with this.
Apr 29 2019
LGTM, but I found one comment a bit confusing for me.
Thanks for the improved commit message. Again, sorry about the delay.
Apr 25 2019
Please add some detail to the commit message and consider re-titling it. It looks like this patch is actually adding missing functionality rather than "fixing" a bug in how structs are handled. If that's correct, then please make the commit message reflect that.
Clever (hopefully not too clever)! Not having to derive a special class from Visitor is really nice.
Apr 24 2019
Can the test deletions be a separate patch, or will they fail because of the other changes in this patch? They feel like a good but separable step.
Apr 23 2019
Nice. Thanks for adding the test, too!
cc: Pavel Labath because I know he's been involved in conversations about how to find symbol files in general (PDB, split DWARF, Breakpad, etc.), especially when doing post-mortem debugging.
Apr 19 2019
Thanks for the changes. If the tests pass, then this LGTM. Just check the one last question I added about the AC1D test.
Apr 18 2019
Sorry for the slow response; I'm still learning about this code.
Apr 17 2019
Thanks for the new test and the test fix. It's unfortunate that the tests are so sensitive to an arbitrary buffer size.
Apr 15 2019
I, too, have some concern that this could have unintended side effects. To make the temporary StringRefs from the zero-terminated strings requires a strlen call each time. So we're making two passes over a string each time (once to measure and once to compare). Granted, these are mostly very short.
Apr 11 2019
I'm even happier. Thanks for giving the params more specific names.
Apr 10 2019
Does this affect any existing tests? Is there a good way to test it?
LGTM, though I'm not clear why this extra visual noise was there in the first place. Does doxygen depend on this style when reading the swigged API?
Apr 8 2019
Overall, I'm ambivalent.
Apr 5 2019
My concerns were address, so LGTM. I'll leave the rest to you and clayborg.
Apr 4 2019
I noticed this also deleted two overloads of Visit from FPOProgramASTVisitorDWARFCodegen, but that appears to be harmless (the base class overloads were also no-ops).