[PATCH] D122179: Serialize PragmaAssumeNonNullLoc to support preambles

[David Goldman via Phabricator via cfe-commits]

dgoldman marked an inline comment as done.
dgoldman added inline comments.


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:436
+  if (PragmaAssumeNonNullLoc.isValid() && !this->PPOpts->GeneratePreamble &&
+      !(CurLexer && CurLexer->getFileID() == PredefinesFileID) &&
       !isEndOfMacro && !(CurLexer && CurLexer->Is_PragmaLexer)) {
----------------
sammccall wrote:
> dgoldman wrote:
> > sammccall wrote:
> > > what's the PredefinesFileID special case about?
> > See ExitedFromPredefinesFile below, this is how I check if we're falling off the end of the preamble region into the main file (same as done for the conditional stack), LMK if there's a better way.
> I don't understand how this checks if we're falling off the preamble region, and the code around ExitedFromPredefinesFile doesn't clarify this for me. Can you explain?
> 
> The `if (ExitedFromPredefinesFile)` appears to be handling the logical *insertion* of preamble PP events when *consuming* a preamble, which is not what you're trying to do here.
> 
> The condition here is of the form "if we have an open pragma, and we're not generating a preamble, and some other stuff, then diagnose". So if the baseline case is "diagnose" and the preamble case is an exception, the "other stuff" clauses don't limit the preamble exception, they add extra exceptions!
`!(CurLexer && CurLexer->getFileID() == PredefinesFileID)` makes sure that if we're consuming the preamble, we don't emit the warning. AFAICT this is the way to tell that the preamble is terminated, since the current file ID being processed is the predefines file and the file is now terminated as of this method call (since !isEndOfMacro && !(CurLexer && CurLexer->Is_PragmaLexer)). `!this->PPOpts->GeneratePreamble` makes sure that if we're generating the preamble, we don't emit the warning. We need to special case both cases otherwise we get an error when generating the preamble or when we load the preamble before even processing the rest of the main file.

Does that makes sense? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122179