- User Since
- Jan 26 2015, 9:26 AM (173 w, 3 d)
Tue, May 15
Mon, May 14
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.
Wed, May 9
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.
Tue, May 8
Mon, May 7
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.
Wed, May 2
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.
Mon, Apr 30
The big picture here is fine.
Looks good to me now, but I'm not an official reviewer.
Fri, Apr 27
This looks fine to me. I just dropped in to add some context on MSVC and __cplusplus: https://blogs.msdn.microsoft.com/vcblog/2018/04/09/msvc-now-correctly-reports-__cplusplus/
Apr 19 2018
Apr 18 2018
I actually preferred the factory solution.
Apr 17 2018
LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.
Apr 16 2018
Apr 13 2018
I meant to mention that the instance of the empty class is reported as 8 bytes of padding (in a 64-bit build), in case that's relevant to anybody's understanding of the issue.
Apr 9 2018
Yes, by all means. My intent was not to block this but to point out that it might still suffer from an already existent problem.
This looks like an improvement, but it still might not work on Windows, because there you're racing the file system--deleting a file on Windows is not an atomic, instantaneous operation. Tune in to https://www.youtube.com/watch?v=uhRWMGBjlO8 at 7:30 for details.
Mar 22 2018
Feb 26 2018
Abandoning. This isn't really working on Windows. The breakpoint fails to set for reasons I don't understand yet.
Nice catch. I'll take a closer look to see what exactly is happening on Windows.
Feb 23 2018
Nit: As a single word, "cleanup" is a noun (or an adjective). As two words, "clean up" is a verb. Given that, I'd expect to see classes and objects with names like Cleanup or cleanup and functions to contain CleanUp. When I see an identifier like CleanUpTest, I expect it's a function that cleans up after a test, not a test of a class called CleanUp.
Feb 22 2018
call_foo1() is defined in foo.cpp, which is built into a DLL, and thus needs the __declspec(dllimport/dllexport) from the LLDB_TEST_API. foo.h defines the API for the DLL.
Right now, we have to add an .exe suffix when using this switch, like -fuse-ld=lld.exe.
This must be new-ish code, since I've routinely used -fuse-ld=lld (without .exe) on Windows.
Feb 21 2018
Please ignore. I'm still trying to figure out arc.
Per Pavel's suggestion, change special value to mean don't check the number of locations found.
By unconditional, do you mean allowing any value for out_num_locations in these cases? I'm happy to do that, but I'm not sure if I've understood you correctly.
Feb 20 2018
Wrapped the modified docstring.
Added new special value to accommodate the fact that Windows may postpone resolving the breakpoints for DLLs that aren't yet loaded.
I have reservations about making the debugger try to pre-locate the modules and their PDBs before those modules are actually loaded. For a few reasons.
Feb 16 2018
OK, so my fix is too simplistic. I'll have to (1) make LLDB on Windows also attempt to pre-load the DLLs, (2) make the test use platform-specific expectations, or (3) use a looser expectation. I'll give it some thought over the weekend.
Feb 15 2018
Excellent patch description! It's reassuring to see all the nagging questions explained. I like the test, too.
Feb 14 2018
Any remaining concerns?
Per Aaron's suggestion, I ran the tests with an unconditional return (and an assertion). There was no change in the test results, so I'm making this return unconditionally.