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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> The patch conceptually models the possible "top-level" statements between two top-level declarations into a block which is emitted as part of the global init function.
> Currently we model this "statement block" as a function body to a void function taking no arguments. We attach this function to the global initializer list to execute the statement blocks in the correct order.

I think some of the tests have confusing behavior based on this conceptual model because the tests do things like declare namespaces and call functions; you can declare a namespace at file scope, not at function scope; you can't call a function at file scope (unless it's part of an initializer). So it the semantics wind up feeling confused because you can't tell whether you're at file scope or at function scope because you seem to be in both at the same time.



================
Comment at: clang/include/clang/AST/ASTConsumer.h:54-59
+  /// HandleTopLevelStmts - Handle the specified top-level statements. This is
+  /// called by the parser to process every top-level Stmt* in incremental
+  /// compilation mode.
+  ///
+  /// \returns true to continue parsing, or false to abort parsing.
+  virtual bool HandleTopLevelStmts(const llvm::SmallVectorImpl<Stmt *> &Stmts) {
----------------
In C and C++, the only thing that's at file scope (that the parser sees) are declarations, not statements, so the phrasing here seems off. I suspect this is because you don't *know* you're only going to get declarations (because it could be that we're trying to parse in the context of an imaginary function body)? If so, perhaps the comments could be updated to clarify that.


================
Comment at: clang/include/clang/Parse/Parser.h:466
+  /// A SmallVector of statements, with stack size 32 (as that is the only one
+  /// used.)
+  typedef SmallVector<Stmt *, 32> StmtVector;
----------------
Drive by punctuation fix. I'm not certain I understand "as that is the only one used", but that's from the original comment. It's possible the comment holds no real weight any longer?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5107-5111
+  static unsigned id = 0;
+  ASTContext &C = getContext();
+  TranslationUnitDecl *TUDecl = C.getTranslationUnitDecl();
+
+  IdentifierInfo *name = &C.Idents.get("_stmts" + std::to_string(id++));
----------------
You should fix the other identifiers in this function to match the usual coding style.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5514-5521
+  // FIXME: this returns true for NS::f();
   // A right parenthesis, or ellipsis followed by a right parenthesis signals
   // that we have a constructor.
   if (Tok.is(tok::r_paren) ||
       (Tok.is(tok::ellipsis) && NextToken().is(tok::r_paren))) {
     TPA.Revert();
     return true;
----------------
rsmith wrote:
> I think we'll need to work out whether we're disambiguating versus a statement here. I think the key observation is that either:
> 
> 1) We know we're parsing a declaration, because we're inside a class body or parsing after `template` or `template<...>` or similar.
> 2) A constructor declaration must be a definition, so if we don't see a `{`, `:`, or `try` after the declarator then it's an expression, not a declaration.
> 
> I'd suggest passing in a flag to say whether we know we're parsing a declaration, which is true unless we're disambiguating against a top-level statement; in that case, don't return here and instead keep going until we hit one of the relevant cases below.
> A constructor declaration must be a definition, so if we don't see a {, :, or try after the declarator then it's an expression, not a declaration.

Consider: 
```
struct S {
  S();
};

S::S() = default;
```
The out-of-line constructor definition is still a kind of declaration despite not ending in `{`, `:`, or `try`.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:54
+    case tok::semi:
+      return true; // Not a stmt but can be extra terminator in `namespace{};`
+    case tok::kw_template:
----------------
http://eel.is/c++draft/dcl.dcl#nt:empty-declaration


================
Comment at: clang/lib/Parse/ParseTentative.cpp:60
+    case tok::identifier:
+      if (getLangOpts().CPlusPlus) {
+        if (isConstructorDeclarator(/*Unqualified=*/false,
----------------
You should assert this at the top of the file; we shouldn't call into here for C code.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:61
+      if (getLangOpts().CPlusPlus) {
+        if (isConstructorDeclarator(/*Unqualified=*/false,
+                                    /*DeductionGuide=*/false,
----------------
Do you need special handling for:
```
struct S {
  operator int();
};

S::operator int() { return 0; }
```


================
Comment at: clang/lib/Parse/Parser.cpp:729-730
 
+  // 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.
----------------
How close are we to having this fixme fixed?


================
Comment at: clang/lib/Parse/Parser.cpp:735
+    ParsedStmtContext SubStmtCtx = ParsedStmtContext();
+    auto R = ParseStatementOrDeclaration(*Stmts, SubStmtCtx);
+    if (!R.isUsable())
----------------



================
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
----------------
Why is this not covered by the REQUIRES above?


================
Comment at: clang/test/Interpreter/disambiguate-decl-stmt.cpp:28-33
+namespace Ns {namespace Ns { void Ns(); void Fs();}}
+void Ns::Ns::Ns() { printf("void Ns::Ns::Ns()\n"); }
+void Ns::Ns::Fs() {}
+
+Ns::Ns::Fs();
+Ns::Ns::Ns();
----------------
This behavior is very confusing to me -- you cannot declare a namespace at function scope and you cannot call a function at file scope, so one of these two seems like it should not work.

(Same kinds of confusion happen elsewhere in the tests.)


================
Comment at: clang/test/Interpreter/execute-stmts.cpp:9-11
+//template <typename T> T call() { printf("call\n"); return T(); }
+//call<int>();
+//// C: call
----------------
Should this code be removed or get a FIXME comment?


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

https://reviews.llvm.org/D127284