- User Since
- Mar 23 2016, 8:38 AM (265 w, 1 d)
If anyone feels like any of the guidelines is actually controversial then let me know and I'll remove it from this review and split it out into its own patch.
- Improve some wording.
I assume you don't have a commit access, so I'm going to land this for you tomorrow. Thanks again for the patch!
The test below reproduces the eager layout generation (and the resulting crashes) for me. Just apply it on top and then this is good to go. Thanks for tracking this down!
Wed, Apr 21
Were you able to reduce the reproducer you got to a test? If it's really the same bug as in D100180 then I guess a similar test case should trigger the issue (e.g., a class with a pointer-to-member member that references some type that is currently being parsed?)
Thanks for updating and thanks for the patch! I can land it right now.
I think this review got kind of stuck. I think the only missing thing is simplifying the self.dbg.CreateTarget call in the test, but otherwise this looks good to me. I'm just going to accept this as the bit of cleanup doesn't need another revision.
Tue, Apr 20
LGTM, thanks for fixing this!
Mon, Apr 19
LGTM. I still kinda like the unique_ptr deleter but let's not block bot-fixes with refactoring requests. I'll open a review for the unique_ptr as a follow up.
Thanks for fixing this, I guess we really need a leak sanitizer/valgrind bot for LLDB...
I think this might have broken the Windows build as it seems the LLDBSwigLuaBreakpointCallbackFunction return type is an error for MSVC (and not a warning which we seem to ignore locally).
Sorry, my bad, I thought if I won't specify any repo/project in Phabricator it won't trigger the Herald (I was using the web interface, not arcanist).
Fri, Apr 16
- Make a variable name more expressive.
Thu, Apr 15
Mon, Apr 12
Fri, Apr 9
- Add missing test case.
Tue, Apr 6
LGTM now beside the now redundant import
Thanks for the patch! I'll be extra nit-picky about the test to make up for the great meme steal of 2021.
LGTM, thanks! FWIW, there are actually a lot more 'qualifiers' supported in Clang that are ignored by the formatters (restrict, Obj-C garbage collector descriptions, custom address spaces, etc.). Most of them are not even modelled in DWARF and users rarely encounter them, so I think const/volatile should cover all reasonable workflows.
Mon, Apr 5
Sun, Apr 4
Thanks for cleaning this up!
Thu, Apr 1
Wed, Mar 31
Not a real review, just dumping code here.
Tue, Mar 30
Mon, Mar 29
- Fixed formatting.
Just queuing this up until the Clang fixes have landed.
It seems Pavel already figured it out, but this is indeed randomly failing on Green Dragon (The bot is green at the moment, but the test usually randomly starts failing again). Not sure what's triggering the failure, but as it's frequently failing on my private bot (Linux) and Green Dragon (macOS), I doubt it's OS related but rather due to both bots being really resource constrained.
So from what I understand this is resolved? If yes, can you abandon/close this revision (You can do this by selecting the "Abandon revision" action above the input field where you can comment).
So something like an ASTConverter that just sits on top of the ASTImporter? In theory that class could implement the ImportImpl callback and do the custom imports it needs to do on its own, so I think that would work. I'm putting this in "planned changes" for now until I get around to make an ASTConverter patch..
LGTM, thanks for the patch (and especially the unit test)! Some two nits left but feel free to fix those when merging. If you don't have commit access I can also do that, just let me know.
Thu, Mar 25
This patch is also intended to get early feedback on how we should approach this. We could also discuss *if* we want to really support this whole cross-language
translation in the ASTImporter, but so far it hasn't been explicitly forbidden so people already started using it this way.
(Marking as requested-changes to get this out of my review queue)
Thanks for the patch!
This still seems to be failing quite frequently on different bots (Linux and macOS):
Wed, Mar 24
The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.
Mar 24 2021
I re-added the llgs decorator in c68a645acb83 because this still seems to be a lldb-server test (which we have disabled on macOS it seems).