[PATCH] D122179: Serialize PragmaAssumeNonNullLoc to support preambles

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:748
 
+TEST(DiagnosticsTest, PreambleWithPragmaAssumeNonnull) {
+  Annotations Header(R"cpp(
----------------
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


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:430
 
-  // Complain about reaching a true EOF within assume_nonnull.
+  // Complain about reaching a true EOF within assume_nonnull unless we're
+  // building a preamble.
----------------
sammccall wrote:
> unless we're hitting _the end_ of the preamble
> 
> This is a special case though, pull it out of the one-sentence summary (as the other special case is)
Done, but isn't it still 2 places - generating the preamble and actually using it both need the exception?


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


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2304
+  // If we have an unterminated pragma assume non null, remember it since it
+  // might be at the end of the preamble.
+  SourceLocation AssumeNonNullLoc = PP.getPragmaAssumeNonNullLoc();
----------------
sammccall wrote:
> this "might" is doing a lot of spooky action at a distance.
> 
> At minimum we should assert this is a preamble, right? Or move this block into the if() below?
Yeah that makes added, added an assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122179