[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

[Aaron Ballman via Phabricator via cfe-commits]

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