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