aaron.ballman added a comment. It looks like precommit CI caught a relevant issue that should be addressed.
Also, there's an in-progress patch that might be worth keeping an eye on in case it changes the behavior for clang-repl in bad ways: https://reviews.llvm.org/D137020 -- the patch causes unknown declaration specifiers to be parsed as if they were a type specifier rather than an expression, which could potentially change repl behavior. ================ Comment at: clang/include/clang/AST/Decl.h:4255 +/// A declaration that models statements on the global scope. This declaration +/// supports incremental and interactive C/C++. ---------------- ================ Comment at: clang/include/clang/AST/Decl.h:4258 +/// +///\note This is used in libInterpreter, clang -cc1 -fincremental-extensions and +/// in tools such as clang-repl. ---------------- ================ Comment at: clang/include/clang/AST/Decl.h:4271-4276 + void setStmts(ASTContext &C, ArrayRef<Stmt *> Statements) { + if (!Statements.empty()) { + Stmts = new (C) Stmt *[Statements.size()]; + std::copy(Statements.begin(), Statements.end(), Stmts); + NumStmts = Statements.size(); + } ---------------- I think we probably want this to `assert(Statements.empty())` rather than silently fail to set the statements. ================ Comment at: clang/include/clang/AST/Decl.h:4274 + Stmts = new (C) Stmt *[Statements.size()]; + std::copy(Statements.begin(), Statements.end(), Stmts); + NumStmts = Statements.size(); ---------------- It's a noop change, but perhaps `std::unitialized_copy` instead? ================ Comment at: clang/include/clang/AST/Decl.h:4287 + FunctionDecl *getOrConvertToFunction(); + ArrayRef<Stmt *> statements() const { return {Stmts, NumStmts}; } + ---------------- Should we add an overload pair here for better const correctness? e.g., ``` ArrayRef<const Stmt *> statements() const { return {Stmts, NumStmts}; } ArrayRef<Stmt *> statements() { return {Stmts, NumStmts}; } ``` ================ Comment at: clang/include/clang/Basic/DeclNodes.td:98 def FileScopeAsm : DeclNode<Decl>; +def TopLevelStmt : DeclNode<Decl>; def AccessSpec : DeclNode<Decl>; ---------------- Oh boy, that node name is a hoot. It ends in `Stmt` but it's a `Decl`. :-D I think it's fine because it matches the convention used for all the other identifiers here, but it may be worth thinking about whether we want a different name for the AST node like `TopLevelPseudoStmtDecl` or `FileScopePseudoStatementDecl`. I don't insist on a change here though, just something to consider. ================ Comment at: clang/include/clang/Lex/Preprocessor.h:1782-1785 void enableIncrementalProcessing(bool value = true) { - IncrementalProcessing = value; + // FIXME: Drop this interface. + const_cast<LangOptions &>(getLangOpts()).IncrementalExtensions = value; } ---------------- We should be able to drop this as part of this patch, right? (I think you can modify the `IncrementalAction` object so that it calls `CI.getLangOpts().IncrementalExtensions = true;` if needed, but you're passing the cc1 flag to the invocation and so I think you can maybe remove this call entirely.) ================ Comment at: clang/lib/AST/Decl.cpp:5244 + llvm::ArrayRef<Stmt *> Stmts) { + assert(Stmts.size()); + assert(C.getLangOpts().IncrementalExtensions && ---------------- ================ Comment at: clang/lib/AST/Decl.cpp:5251 + + TopLevelStmtDecl *TLSD = new (C, DC) TopLevelStmtDecl(DC, BeginLoc); + TLSD->setStmts(C, Stmts); ---------------- ================ Comment at: clang/lib/AST/Decl.cpp:5270-5273 + IdentifierInfo *name = + &C.Idents.get("_stmts_" + llvm::utostr((uintptr_t)this)); + SourceLocation noLoc; + SourceLocation beginLoc = Stmts[0]->getBeginLoc(); ---------------- Some renaming for style, but also, this gets a name that's actually a reserved identifier instead of one that the user could potentially be using. ================ Comment at: clang/lib/AST/Decl.cpp:5276-5277 + QualType FunctionTy = C.getFunctionType(C.VoidTy, llvm::None, EPI); + auto *TSI = C.getTrivialTypeSourceInfo(FunctionTy); + TranslationUnitDecl *TUDecl = cast<TranslationUnitDecl>(getDeclContext()); + FD = FunctionDecl::Create(C, TUDecl, getBeginLoc(), noLoc, name, FunctionTy, ---------------- ================ Comment at: clang/lib/AST/Decl.cpp:5282 + auto StmtArrayRef = llvm::makeArrayRef(Stmts, NumStmts); + CompoundStmt *Body = CompoundStmt::Create( + C, StmtArrayRef, FPOptionsOverride(), beginLoc, getEndLoc()); ---------------- ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6358 + case Decl::TopLevelStmt: { + FunctionDecl *FD = cast<TopLevelStmtDecl>(D)->getOrConvertToFunction(); + EmitTopLevelDecl(FD); ---------------- ================ Comment at: clang/lib/Parse/ParseDecl.cpp:5388-5389 + + // FIXME: Remove the incremental processing pre-condition and verify clang + // still can pass its test suite, which will harden `isDeclarationStatement`. + // Parse a block of top-level-stmts. ---------------- Are you planning on doing this as part of this patch? ================ Comment at: clang/lib/Sema/SemaDecl.cpp:19540 +Decl *Sema::ActOnTopLevelStmtDecl(const SmallVectorImpl<Stmt *> &Stmts) { + TopLevelStmtDecl *New = TopLevelStmtDecl::Create(Context, Stmts); + Context.getTranslationUnitDecl()->addDecl(New); ---------------- Can use `auto` here since the type is spelled out in the initialization (for some reason Phab won't let me suggest an edit here). ================ Comment at: clang/test/Interpreter/disambiguate-decl-stmt.cpp:2 +// REQUIRES: host-supports-jit +// UNSUPPORTED: system-aix +// RUN: cat %s | clang-repl -Xcc -Xclang -Xcc -verify | FileCheck %s ---------------- v.g.vassilev wrote: > aaron.ballman wrote: > > Why is this not covered by the REQUIRES above? > Good question, @sunho, @lhames do you remember if we still need this > exclusion here? > > IIRC, there are platforms which support the llvm-jit but with a limited set > of features, eg. some platforms do not support reliably exceptions, others > relocations (eg some ppc little endian machines). Still wondering on this bit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127284/new/ https://reviews.llvm.org/D127284 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits