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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
