[PATCH] D82118: [clang][module] Improve incomplete-umbrella warning

[Zixu Wang via Phabricator via cfe-commits]

zixuw added a comment.

In D82118#2154280 <https://reviews.llvm.org/D82118#2154280>, @vsapsai wrote:

> Didn't get into details but overall GenModuleActionWrapper approach looks pretty complicated. Haven't tried to accomplish the same myself, so cannot say if such complexity is warranted or not.


Yes the approach is complicated. But one thing to note is that the `GenModuleActionWrapper`-related facilities are upstreamed from apple/llvm-project. I adopted this approach just because it was available. Haven't explored other ways, but yes there might be a simpler approach.

> What happens with fixits in modules when you don't have GenModuleActionWrapper?

If the fixit action is not forwarded to the new compiler instance, it will be handled by the 'outer' compiler instance, which has a different source manager that does not see the module files, so the source location cannot be correctly interpreted. From my experiments, the fixit hint is trying to write into the `<builtin>` file buffer in this test case, when I don't have the action forwarded using `GenModuleActionWrapper`. In this case the fixit rewriter quietly fails because `<builtin>` cannot be rewritten, but since this is not caught I think there might be cases where the wrong file ends up getting updated.

> How does it work with modules disabled? Also looks like there are no tests for incomplete umbrella warning when modules are disabled.

Haven't tested this, will give a try.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82118/new/

https://reviews.llvm.org/D82118