@joerg Yeah, we saw the commit explaining why the original fwd declaration patch was reverted. However, from what I can see, we only have three ways to fix the cyclic dependency between glibc and Clang's internal module:

This looks all good to me and from what see this was requested in a previous review (D44188). Do you need someone to commit this or did you receive commit access?

LGTM modulo the unrelated clang-format changes. Feel free to commit them separately (even though the m_event_names fix looks a bit strange).

Do we also want to get rid of the // C Includes comments that are in some files? LLVM isn't using them either and just removing lines doesn't make git blame more complicated.

LGTM, sorry for that!

Yeah it uses the color settings of the diagnostic engine which are probably set to false in LLDB. I think activating colors there should fix the issue.

I think Pavel's point is to call the dump overload which allows specifying our own custom raw_ostream: https://clang.llvm.org/doxygen/classclang_1_1Decl.html#a278b3b87b6f9d3b20ed566a8684341a6 And our raw_ostream is configured by LLDB to correctly choose if we want color or not.

Minor detail: The revision title only mentions Java, but this revision removes both Go and Java.

This looks nice, just added some minor comments below. Otherwise LGTM after Davide's point is addressed.

Woops, I wanted to accept in my previous comment.

Sorry, this somehow didn't show up in my review queue. I think this can land after two minor things are fixed:

LGTM!

This looks all good to me. I'm a bit worried about the changes to AppleObjCTrampolineHandler.cpp, but from what I can see we don't actually rely on this specific field name anywhere in our code.

• Running settings export /home/teemperor/foobar1 /home/teemperor/foobar2 will actually just create foobar1, but will not report any error that foobar2 is silently ignored.

Runnings settings export crashes LLDB:

Looks good and fixes the tests for me, so I think this is good to go. Thanks for the patch!

Hmmm, maybe I mixing up something here, but the docs state pass_fds was added in 3.2:

Yeah, explicitly typing python2 is what I did to fix it. Not sure if that breaks other OSs though (e.g. if a system has no python2 binary, but only python).

AFAIR, adding an exit(...) to ConnectToRemote won't solve this problem. The test will still be failing on Arch.

Posting my mail here for the record:

Yes, the writeSocketIdToPipe(unnamed_pipe_fd, socket_id); fails in this method/file.

Sure, here is the output: After some debugging it seems that our select wrapper code is failing. We probably could fix this by terminating the process like in the other error cases in the same method (grep for the "failed to write to the unnamed pipe" error to see the code).

I'm accepting this as seems to improve the situation, even though it's not a complete fix.

FYI, for me the test is still hanging on Arch Linux.

Can't you just check for '5', as this is the only information we actually actually care about here?

LGTM minus one small detail (see inline comment). Thanks Vedant!

LGTM now, thanks for the patch!

LibCxx.h/.cpp are still not clang-formatted. I ran clang-format over the patch and pushed it here that this patch doesn't get stuck on this minor detail: https://github.com/Teemperor/lldb/commit/1c5c2f8f5d5af00420559dee52c9155e5bc72518.diff

This test is timing out for me on Arch Linux. Any idea why?

• Rebasing before merging.

• Fixed conflicts before merging.

@jingham I'm confused, what are the requested changes? Because the behavior we seem to agree on is that we keep completions with the same value but different descriptions which is already the current implementation (see lines 138 and 148 in CompletionRequestTest.cpp)

LGTM, thanks!

LGTM otherwise, but don’t know frameworks good enough to accept.

• Removed comment about redeclaring free in the test. That's wasn't correctly formulated and is anyway no longer true now that the test case including this file got bigger.
• Fixed formatting in the free declaration.

(Thanks, Richard).

@davide Maybe git is failing me, but I can only find D47508 but that doesn't seem related. Let's ping @vsk

(Phabricator didn't send my draft, sorry for the delay).

ping :)

In rL341535#287317, @xbolva00 wrote:

Can we rework GetLanguageForExpr? Or at least rename it.

LGTM from my side, but @zturner should take a look too.

(you can also try arcanist which will do this automatically for you)

Please add the header also to the modulemap in Host (which has to be manually written, as the Host module contains a lot of OS-specific headers):
module SafeMachO { header "Host/SafeMachO.h" export * }

Thanks! Didn't know we can activate this setting for the user.

This whole code doesn't seem to have test coverage, so I'll not demand a test for this. Also this patch is clearly fixing a typo, so let's get this in.

LGTM, thanks!

Don't see a way to test this, as this can only be triggered when something in LLDB is going really wrong. That piece of code also has zero test coverage, so I'll just merge it in a few weeks or so unless someone comes up with a good way to test this.

My question about a good test case template was more directed at everyone :)

• Now using a typedef instead of a macro.
• Using LLVM code style in Lazy.h
• Renamed UpdateX to CalculateX