sammccall 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:
> > > 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.
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
