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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D82118#2154310 <https://reviews.llvm.org/D82118#2154310>, @zixuw wrote:

> 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.


My concern about this approach is that it doesn't seem to be composable and propagating specific frontend actions to building modules looks ad-hoc and error-prone. Specifically, from the composition perspective what should we do when both indexing <https://github.com/llvm/llvm-project-staging/blob/f0fd4f3af0acda5068acc5bbe10b3792c41bdfc4/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp#L179-L182> and fix-it are enabled? We can claim they should be mutually exclusive but don't know if it is applicable in general case. From the perspective of propagating other frontend actions to building modules I'm wondering if we need to have custom handling for other actions or not. Personally, I haven't checked that yet.

>> 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.

That doesn't sound good. Though making a source manager support multiple compiler instances doesn't look like an easy task (and maybe not worthwhile).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82118