[PATCH] D122179: Serialize PragmaAssumeNonNullLoc to support preambles

[David Goldman via Phabricator via cfe-commits]

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:
> sammccall wrote:
> > dgoldman wrote:
> > > 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? 
> > > !(CurLexer && CurLexer->getFileID() == PredefinesFileID) makes sure that if we're consuming the preamble, we don't emit the warning
> > 
> > 1) if we're consuming the preamble, what exactly is the sequence of events that would otherwise lead us to emit the warning?
> > 2) what if we're in the predefines file for some other reason instead?
> > 
> > I'm hoping you'll explain to me what you think the predefines file is, and what its relationship to the preamble is, and so why this condition is correct :-)
> > 
> > My understanding (which is pretty shaky!) is that the predefines file is a kind of catchall used to inject text into the preprocessor that doesn't appear in the source file - that it contains definitions of builtin integer types, macros defined by `-D` on the command line, and so on. If it has any relationship to the preamble, it's something subtle that's to do with the relative order in which the preprocessor sees entities like the predefines, the preprocesor, and the main file.
> > So according to my understanding, interpreting "reaching EOF in the predefines file" as "consuming a preamble" is either wrong or something very subtle requiring a significant comment.
> > 
> > The code you refer to below is doing something very different: it's saying that if we have a preamble, then reaching the end of the predefines file is the *trigger* to inject state from it!
> > 
> > > !this->PPOpts->GeneratePreamble makes sure that if we're generating the preamble, we don't emit the warning
> > 
> > Sure, but that doesn't sound correct at all! A preamble mostly consists of parsing a lot of headers, and if any of those headers have an unpaired pragma, we should be warning on that at EOF. It's only if we hit the "pretend" EOF from the truncated main file that we want to suppress the warning.
> > the preprocessor sees entities like the predefines, the preprocesor, and the main file.
> Oops... sees the predefines, any injected events from the preamble, and the main file.
> 
I think that definition is correct, but given that there is only one predefines file, when it ends, it must go into the main file, no? And if we have a pragma assume nonnull loc, it shouldn't be from the clang builtins, it should be from the preamble (I'd certainly hope!)

re: headers, I think there might be a misunderstanding, as soon as an #include is seen the pragma is ended here: https://github.com/llvm/llvm-project/blob/7631c366c8589dda488cb7ff1df26cc134002208/clang/lib/Lex/PPDirectives.cpp#L2006, so if PragmaAssumeNonNullLoc is valid here we haven't had any includes since the pragma was seen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122179