- User Since
- Jan 26 2015, 9:26 AM (220 w, 4 d)
Thanks for the changes. If the tests pass, then this LGTM. Just check the one last question I added about the AC1D test.
Thu, Apr 18
Sorry for the slow response; I'm still learning about this code.
Wed, Apr 17
Thanks for the new test and the test fix. It's unfortunate that the tests are so sensitive to an arbitrary buffer size.
Mon, Apr 15
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.
Thu, Apr 11
I'm even happier. Thanks for giving the params more specific names.
Wed, Apr 10
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?
Mon, Apr 8
Overall, I'm ambivalent.
Fri, Apr 5
My concerns were address, so LGTM. I'll leave the rest to you and clayborg.
Thu, Apr 4
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.
Wed, Apr 3
Nice! Thanks for cleaning up the affected comments as well.
Further simplification per labath's feedback.
Thanks for the review.
Tue, Apr 2
Mon, Apr 1
Tue, Mar 26
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?
Jul 10 2018
Jul 9 2018
Jul 3 2018
Will making it public cause more TUs to transitively include <windows.h>, or does that happen anyway?
Jun 22 2018
I mostly agree with Zach. In practice, setting core.autocrlf to false on Windows seems the most practical solution.
Jun 13 2018
My motivation was not to check for different kinds of errors but to ensure that the error code detail doesn't get lost through the SWIG plumbing. SBErrors seem to be marshaled properly when used as return values, but this is the first one I've seen returned by reference. The test ensures that the success/fail result makes it, but not that the detail (e.g., the error string) survives the trip.
Jun 4 2018
I like the direction. Just a question about the implicit type conversion.
May 31 2018
I was under the impression that some tools rely on the fact that arg is always expanded to an absolute path. Does this work with lldb and its test suite?
May 15 2018
May 14 2018
Thanks for splitting the test.
I kinda wish the test was broken up a bit. The stringtable bundle logic is distinctly different than just applying the keywords to other resource types, so that would be a natural place to break this apart.
Ah, I see you figured out the discrepancy in a follow-on patch (46816), so this LGTM.
May 9 2018
LGTM, but consider inline suggestion.
The docs suggest there are differences between RCDATA and user-defined resource definitions, but the Microsoft rc.exe doesn't seem to follow the docs. For example, the docs say user-defined types can use an external file but that RCDATA cannot. In real life, the resource compiler will accept an RCDATA statement that does reference an external file. Also, the docs say RCDATA can have VERSION, CHARACTERISTICS, and LANGUAGE optional statements, but those don't seem to work because the keywords are mistaken for file names.
May 8 2018
May 7 2018
I'm happy. Thanks!
As I recall, commas may be optionally allowed in several places besides string tables. We'll have to check that and, if necessary, address in a subsequent patch.
May 2 2018
I'm mildly concerned about the tests that give filenames with forward slashes. That doesn't work with a lot of Windows command lines, but apparently the tests here were already doing that.
Apr 30 2018
The big picture here is fine.
Looks good to me now, but I'm not an official reviewer.