LGTM. The new code looks simpler and it's arguably simpler for the clients, since they'll have a consistent set of callbacks independent of the actual view into the lambda expression.

LGTM

The change mostly, the only important comment from me is about toURI. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it.
Could we avoid changing it and focus only on tweaking the "canonical" path for the FileEntries in this change? Those look like a no-op in most practical cases.

Thanks for the update! We still need to solve the technicality here: could you please upload the diff with full context? See LLVM docs for an example on how this can be done with various version controls (the -U99999 args are important, they are the ones that produce diffs with enough lines around it)

Update after investigating with @hokein offline: the RecursiveASTVisitor has custom code to visit lambda parameters and return type, but it fallback to visiting typelocs when both parameters and a return type are specified. Unfortunately this fallback does not work when shouldWalkTypeLocs() is set to false, which is the case for our visitor out here.
It seems reasonable to always visit parameters and return type, rather than relying on traversing the full type-loc of the lamda's function type.

Probably the most important suggestion from my side is trying to clearly separate the enqueue calls (which actually schedule rebuilds of TUs) using clang and the loadShards function.
The index loading should be fast, so I assume we won't win much latency by scheduling the indexing actions in the middle of the shard-loading.
OTOH, separating those two concerns should make the code way more readable (I think, at least).

LGTM with a few nits

LGTM

Good point, we should definitely state it's a clangd-specific extension. We could come up with a naming scheme for our extensions. I would propose something like clangd:$methodName, i.e. the current extensions would be clangd:textDocument/fileStatus.
WDYT?

• Update the test after changes

Thanks for the review!

• Address review comments

LGTM. When we know everyone is happy with the new behaivour, we can remove the flag altogether.

The RecursiveASTVisitor seems to have the code that properly traverses parameters and return types of lambdas. Any ideas why it's not getting called here?

Sorry if I missed any important design discussions here, but wanted to clear up what information we are trying to convey to the user with the status messages?
E.g. why seeing "building preamble", "building file" or "queued" in the status bar can be useful to the user? Those messages mention internals of clangd, I'm not sure how someone unfamiliar with internals of clangd should interpret this information.

Yay! Anything that makes FileManager and friends simpler LG

LGTM. Many thanks for the fix, really important to get those cases right.

In D48116#1322878, @nik wrote:

In D48116#1144732, @ilya-biryukov wrote:

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to filter things.

LGTM

NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.

Maybe try to use Arcanist for uploading diffs?
The web interface can be confusing at times, Arcanist is more reliable (LLVM docs have pointers to the Arcanist documentation to get started)

Ah, sorry, I incorrectly checked that the old revision landed without problems. Have no data on whether the new revision breaks anything, will have to wait for the input from the US timezone buildcop

It looks like the integrate went smoothly with the new change, thanks for fixing it!

LGTM, still have a few NITs and suggestions (see the comment about emitting "queued" on waiting for diagnostics and not emitting queued if the lock was acquired from the first try), but they don't look like something that should block this change.

In D55260#1318280, @kadircet wrote:

I believe also we need another test case where Cls and NamingClass are different.

• Add a newline to the end of the test file

• Reformat

• Make sure we still run ObjC access checks.
• Add a test with a different NamingClass and BaseType.
• Fix IsSimplyAccessible to work in this case.

D55322 is a review for release notes

In D54872#1319684, @malaperle wrote:

It doesn't seem like there is any difference in how -resource-dir and /imsvc are handled: they are all added as -internal-isystem

Thanks for digging this code up.
I'm a bit confused now, will put up a few clarifying questions to make sure I understand the probem properly.
Does clang-cl work correctly the arguments from compile_commands.json? Does it fail if we add the -resource-dir, similar to how it was done for clangd? The compile-commands.json seems to be generated for the MSVC or hand-crafted, I wonder if it may be a difference between the clang frontend and msvc (unlikely, but just to be sure).
The compile_commands.json does not even mention the -resource-dir, I wonder why would adding it break anything? The only thing that comes to mind is some of the microsoft headers being named the same as the clang headers, but having more stuff in them (the uintptr_t that's missing). Is that the case?

LGTM

LGTM with a few NITs.
Also consider adding a test for this.

• Actually use the computed NamingClass

LGTM

Sorry for the delays. Not sure about emitting Queued on the main thread, see the corresponding comment. Otherwise looks very good.

In D54995#1316476, @yvvan wrote:

I don't think removing the flag is a good idea since I can easily assume cases when user wants mmap and is ready to encounter locks. In our case it can be an IDE option which behavior to choose.

Would that option be useful if changing it forces the files user is editing in the IDE to be indefinitely locked and stops them from saving the files?
Anyway, even if you feel this behavior should be configurable, that's totally fine - I was referring to removing the flag from the vfs::FileSystem interface, possibly replacing it with a filesystem implementation that would handle the same thing at a lower level.
This flag is not useful to any of the filesystem implementation, except RealFileSystem, which actually has to deal with opening OS files.

Everything looks good, just a single important request about testing the included files do not have any edges in the resulting graph

In D54630#1315168, @arphaman wrote:

So far everything looks fine, but I have to rerun a couple of things due to infrastructural issues. I should have the final results next Monday.

• s/VolatileFSProvider/VolatileFileSystem

• Keep using memory-mapped files for PCHs
• Move once

The idea of using the AST-based approach here was really nice, it was less expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the source locations.

Thanks for taking a look at this.

In D54995#1316437, @yvvan wrote:

Ok, no global option.
Why not placing your VolatileFSProvider in clang so that libclang could you it too?

It seems there are still cases where memory-mapping should be preferred, even on Windows, specifically:

• PCH files produced by libclang: they are huge, loading them in memory is wasteful and they can clearly be used
• (maybe?) PCM (precompiled module) files: they are huge, however can be produced by the user's buildsystem. Locking them might be bad if we don't control the buildsystem, but the memory savings are attractive.
• other binary input files?

We need to keep mmapping the PCH files.

• Do not introduce a new local var, reuse existing

In D54995#1315024, @zturner wrote:

We can work around it by reading the whole file, but it looks like a bug in QtCreator to me. We have the file mapped, but maybe when they open the file to save it, *they* are opening the file without FILE_SHARE_READ.

I haven't tried QtCreator, but this also happens in VSCode and Vim. I suspect other editors are also affected, but I haven't checked.
I looked at the logs in Process Monitor and I think they do set the share flags properly (~95% certain, will still have to double-check). Even if they do it wrong, it will take time to fix this and I don't think we can afford breaking users with existing editors in our tooling.

In D54995#1314912, @zturner wrote:

Original patch description says this:

There are reported cases of non-system files being locked by libclang on Windows (and likely by other clients as well)

What is the nature of the reports? What operation is attempted on the files and fails due to locking? And which application is it that's failing and in what way?

https://bugreports.qt.io/browse/QTCREATORBUG-15449 was mentioned previously in the thread.

In D54995#1312617, @lebedev.ri wrote:

Passing-by thought, feel free to ignore: this seems to so far only affect windows only?
So the fix shouldn't probably pessimize all other arches? (and maybe even non-clangd)

• Fix a comment

• Add missed field init
• Also fix access checks for member access with qualifiers

@arphaman, did you have a chance to run the tests? There's not rush, just wanted to know whether we have any data at all at how

Thank, LGTM. A few NITs.

LGTM and a few nits

@ilya-biryukov, I hope the current patch is not too big for you to review, happy to chat offline if you want (sam and I had a lot of discussions before he is OOO).

Picking it up. Mostly NITs from my side and a few questions to better understand the design. Also happy to chat offline.

In D55052#1312766, @hokein wrote:

For cancellation errors, it might be reasonable, but what if other unrecoverable errors in clangd (e.g. clangd crashes), in these cases, users don't know what happens, and would still expect IDE features (like code completion) to work...

Yeah, there are still modes where clangd behaves badly there. Note that the output from that pane is rarely useful anyway as clangd keeps producing errors about accessing non-open files, which would confuse people even more.
Overall we never designed this output to be readable by the users, so it's not really useful anyway. There are ways to communicate with the users in the protocol, we should be using them instead to be more user-friendly

+1 to the change, this is annoying for me too.

In D54995#1311427, @zturner wrote:

My first thought would be that it depends on the flags to CreateFile moreso (and perhaps entirely) rather than the flags to MapViewOfFile or CreateFileMapping. Specifically, the FILE_SHARE_XXX flags are the most relevant here.

That was my initial thought as well and it worked fine on Windows Server 2012. Will have to do it again to double-check, though, not sure I can dig up that code.

In D54995#1311303, @yvvan wrote:

"could we figure out the exact cause of the issue?"
And this review was not meant to proceed immediately but rather state the intention and get some feedback because I still don't know answers to the questions 1 and 2 (did somebody else investigate that?)

I tried calling Windows APIs that LLVM uses with multiple combinations of flags and couldn't make it lock the file. But maybe I didn't arrive at the right combination of flags or was trying on a different version.

cross-posting @zturner's comment from the mailing thread for the record:

I’m afraid this is going to be too severe a performance regression if we change the default. So I don’t think this should be an LLVM-wide default