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