@sammccall RE using USRs: we want to integrate clangd into a larger source tooling system that is already using USRs extensively to identify symbols. The most obvious case is that we have an index outside of clangd that uses USRs from clang-the-compiler, so exposing USRs in clangd lets us integrate information from both places. The most critical piece of this is being able to ask "what symbol is at this location" and correlate that with index queries. Beyond this one case, my experience is that any time a language service API is providing information about a symbol or list of symbols it may be useful to add the USR (e.g. hover, definition or workspace symbols).

LGTM

LGTM

r333202

Thanks for looking up the supported bash version! Updated diff.

In D47273#1109985, @ruiu wrote:

sed -e "$(echo -e 's/\t.*//')"

In D47273#1109934, @ruiu wrote:

I wonder if you could replace \t with \0x09. At least it works on my machine which has GNU sed.

r328337

Did you consider just using a different pragma that triggers this behaviour instead of avoiding the crash? I don't really have a strong preference but I'd be interested to hear what you think the pros/cons are.

Spotted one issue, but otherwise seems okay to me. I'd like @bruno to take a look at this, because he's thought about this code much more recently than me.

In D34512#856301, @xazax.hun wrote:

In D34512#856184, @dcoughlin wrote:

In either case, the important scenario I think we should support is choosing at a call site to a C function the most likely definition of the called function, based on number and type of parameters, from multiple possible definitions in other translation units. If the API is rich enough to support this then I think that is a good indication it will be useful for other scenarios as well.

Note that the lookup is already based on USR which is similar to mangled names in a sense that it contains information about the function parameters. So the only way to get multiple candidates from the lookup is having multiple function definitions with the same signature.

Thanks, this looks good to me. I'd appreciate if @klimek could take a quick look though.

It would be nice if the doc comment for the single file parse mode flag was updated to include this new functionality.

LGTM, thanks!

I agree with not detecting these during PP-only, but there's nothing wrong with <#>. It's either not a placeholder, or it's part of a placeholder like <#>#>, which is a placeholder containing the text ">". Similarly, <## could be the start of an empty placeholder <##>.

LGTM. One stylistic comment, but I'll leave that up to you.

A couple of minor comments, but otherwise LGTM.

What do you think about a hybrid approach: Don't suppress diagnostics in the placeholder range when we handle the placeholders in parser/sema, but do suppress the range when the placeholder isn't explicitly handled?

LGTM

Rather than stick ranges into the diagnostic engine, I think it would be cleaner to have the identifier have a method like isEditorPlaceholder() that can be used to avoid situations like this in a principled way, or to customize behaviour for placeholders in the parser, etc. That's how we are handling it in Swift. Using an API on the placeholder is also better for handling errors that could be caused by the placeholder but not have it as the primary location.

LGTM

Sorry for the delay, I though you were still iterating with @aprantl for some reason. LGTM

LGTM

Can we test the already-validated diagnostics?

I'm inclined to suggest we break that behaviour with modules and treat a user module the same no matter where it was imported. @rsmith, what do you think?

Does it mean that a system module should only import system modules? If a system module is allowed to import non-system modules, for a non-system module, we will validate diagnostic options differently depending on whether a system module or a non-system module imports it. This will cause a non-system module that was validated earlier to be invalidated by a child thread.

LGTM, sorry for the delay.

LGTM, but @rsmith should glance at this too.

This should have a test for the writer change. Otherwise LGTM.

Still missing the change for if (IsPrebuilt) {, but otherwise LGTM.

Sorry for the delay, I apparently forgot to hit submit. Replied inline.

How about -fmodules-use-prebuilt-module-cache for the flag name? Saying "prebuilt-modules" is confusing to me, since -fmodule-file can also be used to load a prebuilt module, but doesn't use a cache.

LGTM

the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options

"a compilation that doesn't use a PCH", is that a different project? And we have two projects building in parallel? Just to make sure I understand.

We need to add this option to the module hash (see getModuleHash - we already add a bunch of other HSOpts there). Otherwise the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options.

Can you point me to the source codes where we use rename to replace the file? I am curious on how this all works out.

B.pcm becomes out of date when we try to load module "A", because another instance overwrites "B.pcm"

LGTM

LGTM. Some minor suggestions.

I'd like to see Doug and/or Richard review this. It seems reasonable to me to first blush, but I assume there was a good reason we weren't doing this already...

LGTM

LGTM

Let's move the code looks up the alternate name out of the ModuleMap parser, and into the dependency collector callbacks. This feels like an implementation detail of the dependency collector. We could add a new callback specifically for umbrella headers and handle this inside there.

LGTM with one comment about the doxygen comments, but you should probably wait to hear from @akyrtzi too.

Overall approach seems good to me, but this needs tests. Other feedback inline.

From what I understand from http://lists.llvm.org/pipermail/cfe-dev/2014-July/038273.html, changing the behavior of the cached result has side-effects and demands a non trivial change, am I assuming the right thing?

This doesn't appear to be a safe change. We can't assume that RootName wouldn't ever be legitimately found somewhere in a path (even without a VFS) and changing the path prefix could give a completely different location. I would also be concerned about the performance impact of performing two lookups, since finding a module for a header is not always a cheap operation.

LGTM

This LGTM, but I would like Richard to take a look too.

LGTM!

LGTM if you fix 256->PATH_MAX.

Okay, let's go with the two-path solution. Thanks!

Given the two-path solution, another thing we could do in the first path, is to default to the remove_dots() version for the source, i.e. "/install-dir/lib/clang/3.8.0/include" instead of "/install-dir/bin/../lib/clang/3.8.0/include". The path after remove_dots() might be different from what realpath() would return (but this is covered in the 2nd path) but at least we canonicalize for a future virtual path request (which can safely request the VFS based on the returned path from remove_dots()). What do you think about it?

Now, we try to use the new YAML file + cache from a new clang invocation. When the FileManager requests status in (2) for the path "/install-dir/bin/../lib/clang/3.8.0/include/altivec.h" neither "remove_dots()" nor "realpath()" would be able to resolve it to "/private/tmp/a/b/lib/clang/3.8.0/include/altivec.h" and will fallback to search the real filesystem instead, if this is a different machine, it will fail. Since we are looking at a virtual path in (2) it could makes sense to use remove_dots() but it won't make sense to use "realpath()".

How do you propose we fix that?

I don't think this is the right approach. If we don't canonicalize the source path then:

• looking up the path *without* the .. won't work, which means anything that looks up a realpath will fail
• directory iteration won't combine entries from foo/bar/.. and foo/

Please clarify what you mean here:

The rationale is that if source and destination paths in the YAML file contain ".." this is enough
for the file manager to retrieve the right file, meaning that it doesn't matter how we write it
since the FileManager is capable of transforming it in real/feasible paths.

Any regression to code completion was below the noise level in my testing.