- User Since
- Jan 26 2015, 9:26 AM (230 w, 1 d)
The LGTM, but I wasn't nominated as a reviewer, and I was mostly looking at it from the point of Windows compatibility.
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.
Tue, Jun 18
Mon, Jun 17
Fri, Jun 14
I'm OK with moving common stuff out for now, and I like the separation of ProcessWindows and ProcessDebugger.
Thu, Jun 13
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.
Tue, Jun 11
Sorry for the stupid question, but ...
Mon, Jun 10
Fri, Jun 7
Changed the approach in the first fix to use explicit escaping after Joel Denny's comment got me to re-think it.
Tue, Jun 4
Fri, May 31
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).
This looks good. I have a few questions inline, but none of those are major concerns.
Apr 3 2019
Nice! Thanks for cleaning up the affected comments as well.
Further simplification per labath's feedback.
Thanks for the review.
Apr 2 2019
Apr 1 2019
Mar 26 2019
The Windows code looks correct to me, but I've added a couple inline questions/comments.
Mar 12 2019
Mar 6 2019
Feb 28 2019
Closed with r355121.
Thanks. I really don't know what other types of conditions may trigger that "request is not supported" message, so I'm going to shy away from trying to make it more specific.
Feb 27 2019
Feb 25 2019
Forgot to include context in original diff.
Feb 11 2019
My knowledge of PECOFF is even more limited, but it's my understanding that the ImageBase is the _preferred_ address for the module. That doesn't guarantee that's the actual address it would be loaded at. Not just because of ASLR but also because there may be conflicts with modules that have already been loaded. Is GetBaseAddress supposed to return the actual base address or the preferred one?
Feb 8 2019
I think absolute_path is great. Thanks for checking on the native/None. I didn't want there to be any latent surprises.
Feb 7 2019
Feb 5 2019
Jan 10 2019
Jan 9 2019
So this Patch is effectively NFC, since no caller (not even a test) was using the functionality you've removed. Seems like a nice refactor.
Jan 8 2019
Dec 6 2018
If there is a magic value, should we be checking it?
Nov 30 2018
This looks good to me, save for a couple comment nits.
Jul 19 2018
Jul 12 2018
LGTM if you don't want to consider my remaining suggestion for this patch.
Jul 11 2018
Also, I'm not seeing tests for the other consistency checks you're adding (like whether there are any streams or whether the streams overlap). Is it difficult to create such malformed minidumps?