fun fun fun fun fun

lgtm too

lgtm

lg

lgtm

lg

This looks good to me.

I think this can be committed now. It's still a bit awkward but we can address that later.

Changes like this should come with a small, c-index-test based, test case so we don't reintroduce the same bug in the future.

lg

Test case?

There's some similar code in tools/clang/lib/Tooling/Core/Lookup.cpp, it might make sense to share it. Otherwise this looks good.

lgtm

looks good

ship it!

lg (it could use a test case though)

lg under the precondition that clang-tidy tests still work.

lg

Looks good.

This is fine. We should fix phab though.

This is fine. We should fix phab though.

This fixes the issues I'm seeing.

Looks good, but I'm wondering if we should also change the default in clang-apply-replacements itself.

lg

Looks good. Make sure to watch the windows buildbots after submitting.

lg

looks good to me

This makes sense to me.

lg

Kill it :)

looks good, thanks!

I believe this is good enough now.

looks good, thanks!

Can you please run clang-format on this change? There are pieces that don't follow the style.

Still looking good. Will commit this soon.

lg from my side. It would be good to wait until Manuel is back though, I think he had plans for extending this interface at some point.

lg

This is fine. Test case would be nice though.

A test case would be nice.

lg

lg

This is fine.

Let's fix those crashes one at a time. This patch looks good, only minor comments below.

kill it with fire

libclang is supposed to be used for bindings to other languages (it's a pure C interface) and its stable. ASTUnit is the C++ side of libclang, libclang is just a thin wrapper around ASTUnit. ASTUnit itself suffers from lots of technical debt because clang changed since ASTUnit was created and ASTUnit was never refactored to accommodate that.

It's not. the goal is to get rid of ASTUnit inside of clangd in the long term as it's a big problem for extensibility. libclang is just a wrapper for ASTUnit, with even more problems.

lg

lg, do you have commit access?

lg as a prototype.

lg

Generally makes sense. Is there any reason for the #ifdef? Windows has errno and EINTR too.

I assume this is fine but I don't really understand what's going on. A test case would be great.

• Use typedef instead of decltype()
• Rename local variable not to shadow member.
• Give FixIts their own mutex
• Use const instead of let in typescript

This will get you an unused function warning if NDEBUG && LLVM_ENABLE_DUMP. You can use LLVM_DUMP_METHOD to avoid that.

lgtm

lg

lg

lg, but I don't really know JS ;)

lg

• Inline the request struct again.

• Do not lock while running DocumentStore callbacks
• Replace fake queue with a real queue (but it still has at most one element.

• Address review comments.

lg

• Simplify code by not worrying about \r
• Don't allow blank lines between NOLINTNEXTLINE and the warning

Address review comments. Make test actually run (missing cmake file)

In D29451#667606, @arphaman wrote:

This might be a bad question, but is there any particular reason why you didn't use the YAML Traits API for parsing instead of the raw YAML Stream API? In my experience the traits based API is quite convenient for structured data like various options and parameters in this protocol.

lg

lg

lg

Your test case is fine, it crashes with assertions enabled.

lg

Only nits below.

In D29124#656224, @rengolin wrote:

Though, hum, hard to make tests for that...

lg

lg

lg. I assume the test passes with the RUN: false removed?

This looks useful to me.

lgtm

can we just use support::endian::read32le and support::endian::read32be instead of the casting magic? They do the same thing but should have no problem with implicit conversions in a ternary expression.

The reason why we disallowed specializations is that there are type-trait classes that get specialized all over the place (DenseMapInfo in LLVM is one example). I think we can get away with allowing partial specializations though. Those should be much less common and we can give the user a choice between the headers that contain them.