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

[Volodymyr Sapsai via Phabricator via cfe-commits]

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: clang/lib/Lex/PPLexerChange.cpp:266
 void Preprocessor::diagnoseMissingHeaderInUmbrellaDir(const Module &Mod) {
-  assert(Mod.getUmbrellaHeader() && "Module must use umbrella header");
-  SourceLocation StartLoc =
-      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
-  if (getDiagnostics().isIgnored(diag::warn_uncovered_module_header, StartLoc))
+  const auto &UmbrellaHeader = Mod.getUmbrellaHeader();
+  assert(UmbrellaHeader.Entry && "Module must use umbrella header");
----------------
I was going to suggest to spell out the type explicitly instead of `auto` because I wasn't able to guess the type. But when I've checked, `Module::Header` didn't look particularly better. I'll leave it up to you to decide which one is more readable, for wider context there was a discussion [RFC: Modernizing our use of auto](https://groups.google.com/forum/#!topic/llvm-dev/GSyITfY1BLg).


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:269
+  const FileID &File = SourceMgr.translateFile(UmbrellaHeader.Entry);
+  SourceLocation EndLoc = SourceMgr.getLocForEndOfFile(File);
+  if (getDiagnostics().isIgnored(diag::warn_uncovered_module_header, EndLoc))
----------------
Can we rename `EndLoc` to better convey its purpose, not how it was computed? I was thinking about something like `SuggestedHeadersLoc` or `ExpectedHeadersLoc`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82118