In D148997#4561620, @v.g.vassilev wrote:

So, in that case we should bring back the boolean flag for incremental processing and keep the IncrementalExtensions LanguageOption separate. In that case IncrementalExtensions would mean that we must turn on incremental processing for managing lifetime and only use the language option when extending the parsing logic. However, I think the problem would be what to do with the tok::eof and tok::annot_repl_input_end? I'd probably need @aaron.ballman or @rsmith here...

In D148997#4561577, @v.g.vassilev wrote:

My other concern here is that it seems our use cases are somewhat different, eg. we wouldn't want any parsing differences and while I don't know why this is yet, the removal of:

I think I understand now. We basically want a mode which keeps some of the clang objects alive and ready to process more input. And on top, for clang-repl we want to enable some special parsing mode for the top-level statement declarations.

In D148997#4561211, @v.g.vassilev wrote:

https://github.com/llvm/llvm-project/blob/df6b35e329ebecad6dc3bfb83183e482eb7a0020/clang/lib/Parse/ParseExprCXX.cpp#L4070

That looks a bit obscure to me. Looks like we are trying to reach some error recovery anchor but do you happen to have some use case at hand? In principle we could do the same as for the other 3.

This was just one I picked at random from my grep over the parser. It's unclear how often this would be hit, but I have to assume it (and the other references) can, even if they're obscure/unlikely. My main point is that !eof is being used somewhat frequently to end loops, but with this change they will now all never end.

Since eof there were several eof-like tokens that were added: annot_module_begin, annot_module_end, tok::annot_module_include which are commonly handled in isEofOrEom. We could update these uses with isEofOrEom but I am pretty sure that @rsmith will point out cases where that's not a good very good idea :) We could either experiment with using isEofOrEom or have a similar utility function only for the two tokens (say isEoI -- end of input whatever that means).

In D148997#4561022, @v.g.vassilev wrote:

I meant that I'd like to figure out if we could use the annot_repl_input_end before considering a new flag.

In D148997#4559788, @v.g.vassilev wrote:

I'd prefer to avoid adding a new flag. Is there a way to see how does the diff looks like?

Are there other users of incremental processing mode, other than the REPL / IncrementalParser?

I have decided against doing that, because we can't specify @LINE in the c-index-test command line.

LGTM, thanks Alex!

LGTM. If you could put up a PR after to fix the use of sys::fs::make_absolute that would be appreciated 🙇.

Add missing --target

This seems reasonable to me in general. @dexonsmith in case you have any thoughts.

Thanks Egor!

You're correct that this overhead has been measured on implicit module builds. As I mentioned in the commit message this saves over 20% of the overall built time in some cases. This time is split between module validation (which could be skipped) and HeaderSearch (which cannot be skipped).

Mostly just skimmed so far, will hopefully have some time to look at this more tomorrow. Out of interest, do you have any performance numbers using this change? I assume this mainly impacts implicit modules (since I suspect we'd also be opening the file as well anyway), is that true?

In D135634#3866704, @jansvoboda11 wrote:

In D135634#3866353, @benlangmuir wrote:

I think we should deduplicate the vfs overlays if the same ivfsoverlay is specified in both the pcm and the command-line.

My understanding is that ASTUnit never uses command-line and always adopts whatever is in the PCM.

In D135634#3866353, @benlangmuir wrote:

I think we should deduplicate the vfs overlays if the same ivfsoverlay is specified in both the pcm and the command-line.

@bnbarham any concern about overlay vs chaining behaviour here? I remember you looking at that a while ago.

When we try to load such outdated .pcm file, we shouldn't change any already loaded and processed modules.

Thanks 🙇

LGTM, thanks for doing this 🙇

The failure here is likely due to the hack in FileManager::getFileRef:

// FIXME: This hack ensures that `getDir()` will use the path that was
// used to lookup this file, even if we found a file by different path
// first. This is required in order to find a module's structure when its
// headers/module map are mapped in the VFS.

So the directory returned from the FileEntry is the *lookup* directory, but the FileEntryRef will be giving back the directory from the external path (if it has been remapped).

Failing test seems to be because . is turned into the full path at some point. It's possible that this is because getFileRef returns the absolute path (see the massive FIXME there). If that's the case we could fix that by fixing clang-apply-replacements and then change the Status.getName() == Filename to only check whether it's an externally mapped path.

Few small comments, LGTM otherwise.

Remove line for <unknown> check

Update title/message

After speaking with Ben, we decided it makes more sense to just remove the reference entirely.

Shadowing of FE almost tripped me up there 😅

Thanks for doing all these Jan!

Thanks Jan :)!

Does clang/test/VFS/external-names-multi-overlay.c need to be formatted or is it fine? It uses split-file so I'd really like to avoid the format here (makes it pretty silly).

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

Added a potential plan to remove the FileManager hacks.

This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding ExposesExternalVFSPath instead of replacing IsVFSMapped.

Keep using the redirection hack for the time being