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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGed79827aea44: [clang][module] Improve incomplete-umbrella warning (authored by zixuw). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. Looks good to me. I would wait a few days before committing in case other reviewers have comments but not too long as you can address extra comments post-commit. Repository: rG LLVM

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

zixuw updated this revision to Diff 292058. zixuw added a comment. Address review: - Use explicit type name `Module::Header` instead of `auto` since it's a short typename and using `auto` does not improve readability; - Change variable `EndLoc` to `ExpectedHeadersLoc` so it's self-explanatory.

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

vsapsai added a comment. Noticed that the warning and the fix-it might not work well with pragmas suppressing diagnostic and with header guards. But it's not a regression and I don't think it is worth improving these use cases preemptively. Comment at:

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

zixuw added a comment. @arphaman @vsapsai @bruno This diff has been sitting here for too long. I've separated out the fix-it hint part (working on implementing a //'FullSourceRange'// to be used by FixItHint now) and this is just a concise change for the warning message now. Does it look

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

zixuw updated this revision to Diff 291364. zixuw added a comment. Separate warning message improvement and fix-it hint. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82118/new/ https://reviews.llvm.org/D82118 Files:

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

zixuw added a comment. Okay so I think I traced down to the root problem: the diagnostic message prints out fine because `TextDiagnosticPrinter` uses `FullSourceLoc`, which actually embeds an appropriate `SourceManager` within it: TextDiag->emitDiagnostic(

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

bruno added a comment. > Yes the wrapper is definitely problematic. I'm checking how the diagnostic > itself is handled correctly and start from there when I have time. For now > would it be better to separate this into multiple patches and get the > diagnostic improvement in first? Yep,

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

zixuw added a comment. Hey Bruno! Thanks for the review. In D82118#2224423 , @bruno wrote: > Hi Zixu, thanks for working on improving this. > > I agree with @vsapsai on the the `GenModuleActionWrapper` approach. Also, it > seems to me that even though

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

bruno added a comment. Hi Zixu, thanks for working on improving this. I agree with @vsapsai on the the `GenModuleActionWrapper` approach. Also, it seems to me that even though it would somehow improve the accuracy, it would be solving a more general problem, not really specific to this patch.

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

vsapsai added a comment. In D82118#2154310 , @zixuw wrote: > In D82118#2154280 , @vsapsai wrote: > > > Didn't get into details but overall GenModuleActionWrapper approach looks > > pretty complicated. Haven't

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

zixuw added a comment. In 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

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

vsapsai added a comment. 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. What happens with fixits in modules when you don't have

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

zixuw added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82118/new/ https://reviews.llvm.org/D82118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

zixuw created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. - Upstream methods for action wrappers in generating modules; - Change the warning message for -Wincomplete-umbrella to report the location of the umbrella header; - Add a fix-it hint to include