[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:748
 
+TEST(DiagnosticsTest, PreambleWithPragmaAssumeNonnull) {
+  Annotations Header(R"cpp(
----------------
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
```


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


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:775
+  TU.AdditionalFiles["header.h"] = std::string(Header.code());
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
----------------
you verify no diagnostics, but likely also want to verify that the parameter has the nonnull attribute.

This is probably simple enough, e.g 

```
ParsedAST AST = TU.build();
ParmVarDecl *X = cast<FunctionDecl>(findDecl(AST, "foo")).getParamDecl(0);
ASSERT_TRUE(X->hasAttr<NonnullAttr>());
```

(or by matching the AST dump)


================
Comment at: clang/include/clang/Lex/PreprocessorOptions.h:131
   /// conditional #if stack, so the ASTWriter/ASTReader can save/restore it when
-  /// processing the rest of the file.
+  /// processing the rest of the file. In addition, for preambles, we keep track
+  /// of an unterminated pragma assume nonnull.
----------------
nit: "Similarly, we track an unterminated #pragma assume_nonnull"?

Currently it's not completely clear what the connection is.


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


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:435
   // instantiation or a _Pragma.
-  if (PragmaAssumeNonNullLoc.isValid() &&
+  if (PragmaAssumeNonNullLoc.isValid() && !this->PPOpts->GeneratePreamble &&
+      !(CurLexer && CurLexer->getFileID() == PredefinesFileID) &&
----------------
It's not valid *anywhere* we exit a file when building a preamble, only when we fall off the end of the preamble region itself within the main file.


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:436
+  if (PragmaAssumeNonNullLoc.isValid() && !this->PPOpts->GeneratePreamble &&
+      !(CurLexer && CurLexer->getFileID() == PredefinesFileID) &&
       !isEndOfMacro && !(CurLexer && CurLexer->Is_PragmaLexer)) {
----------------
what's the PredefinesFileID special case about?


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2303
 
+  // If we have an unterminated pragma assume non null, remember it since it
+  // might be at the end of the preamble.
----------------
nit: #pragma assume_nonnull


================
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();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122179