[PATCH] D122179: Serialize PragmaAssumeNonNullLoc to support preambles

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:755
+)cpp");
+  TU.Filename = "foo.m";
+  auto AST = TU.build();
----------------
nit: not necessary, this pragma isn't tied to objc


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:748
 
+TEST(DiagnosticsTest, PreambleWithPragmaAssumeNonnull) {
+  Annotations Header(R"cpp(
----------------
dgoldman wrote:
> sammccall wrote:
> > sammccall wrote:
> > > This test seems to have a lot of extraneous elements.
> > > 
> > > The following seems sufficient, with no header:
> > > ```
> > > #pragma clang assume_nonnull begin
> > > void foo(int *x);
> > > #pragma clang assume_nonnull end
> > > ```
> > this should be a clang test, rather than a clangd test (or in addition)
> > 
> > probably under clang/PCH/, of the form
> > ```
> > // RUN: %clang_cc1 -emit-pch ...
> > // RUN: %clang_cc1 -include-pch -fsyntax-only -verify -ast-dump ...
> > // and optionally without PCH:
> > // RUN: %clang_cc1 -fsyntax-only -verify -ast-dump ...
> > ```
> > 
> > Then you can verify both the lack of diagnostics and the AST that gets generated.
> Hmm I tried this earlier, maybe I did it wrong, but I don't think it works since it doesn't set the `GeneratePreamble` bit
(please don't mark comment threads done unless they've been addressed)

You're right, regular PCHes are not preambles and so don't handle these cases.
Probably the best place then is something using c-index-test similar to `clang/test/Index/preamble-conditionals.cpp`


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:436
+  if (PragmaAssumeNonNullLoc.isValid() && !this->PPOpts->GeneratePreamble &&
+      !(CurLexer && CurLexer->getFileID() == PredefinesFileID) &&
       !isEndOfMacro && !(CurLexer && CurLexer->Is_PragmaLexer)) {
----------------
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!


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2303
 
+  // If we have an unterminated #pragma assume_nonnull, remember it since it
+  // should be at the end of the preamble.
----------------
it should be -> we should be
the preamble -> a preamble
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122179