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

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a subscriber: sunho.
v.g.vassilev added inline comments.


================
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) {
----------------
aaron.ballman wrote:
> 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.
I am a bit confused. Could you clarify what wording changes you had in mind?


================
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;
----------------
aaron.ballman wrote:
> 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?
It does not indeed carry weight. Let me try to improve it.


================
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++));
----------------
aaron.ballman wrote:
> You should fix the other identifiers in this function to match the usual coding style.
I have not paid too much attention in the coding style here as it was mostly for initiating a design discussion.

The body of this function breaks the design of clang which assumes AST to be created by the frontend and be essentially immutable in `CodeGen`. This `FunctionDecl` needs to be treated as a global initializer. That is, it needs to be run in a particular order to the other initializer following the lexical occurrence of the statement block. That leaves us with two options, either to expose the registration of global initializers outside of CodeGen, or we need to create that here.

Alternatively, we could make `HandleTopLevelStmts` take a `FunctionDecl` and we can create it in the `IncrementalParser`. That blurs the responsibility and coherence of the interfaces. We will have public interface `HandleTopLevelStmts` (or similar) which handles top-level statements but take a `FunctionDecl` as a parameter which is created by the user...


================
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;
----------------
aaron.ballman wrote:
> 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`.
That came up a lot in the testsuite but I've added a test for it.


================
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:
----------------
aaron.ballman wrote:
> http://eel.is/c++draft/dcl.dcl#nt:empty-declaration
IIUC, clang models the `;` as a `NullStmt`.


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


================
Comment at: clang/lib/Parse/ParseTentative.cpp:61
+      if (getLangOpts().CPlusPlus) {
+        if (isConstructorDeclarator(/*Unqualified=*/false,
+                                    /*DeductionGuide=*/false,
----------------
aaron.ballman wrote:
> Do you need special handling for:
> ```
> struct S {
>   operator int();
> };
> 
> S::operator int() { return 0; }
> ```
We seem to need that. Thanks. I've added a FIXME.


================
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.
----------------
aaron.ballman wrote:
> How close are we to having this fixme fixed?
2000 tests away. The tests are mostly in OpenCL, OpenMP, etc, which just need adding more entries in places such as `isCXXDeclarationSpecifier`. I've decided to keep the patch small and then add that once we merge this one.


================
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
----------------
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).


================
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();
----------------
aaron.ballman wrote:
> 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.)
The mental model has been that we have a mix of statements and declarations on the global scope. The sequence of statements are only considered to be as if they have been wrapped in a function body. For example:

```
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();
```

should be semantically equivalent to:

```
namespace Ns {namespace Ns { void Ns(); void Fs();}}
void Ns::Ns::Ns() { printf("void Ns::Ns::Ns()\n"); }
void Ns::Ns::Fs() {}

void stmt_block1() {
  Ns::Ns::Fs();
  Ns::Ns::Ns();
}
auto _ = (stmt_block1(), 1);
```

Does that help in making it less confusing when thinking about it?


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

https://reviews.llvm.org/D127284