[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: akyrtzi.
v.g.vassilev added inline comments.


================
Comment at: clang/include/clang/Basic/DeclNodes.td:98
 def FileScopeAsm : DeclNode<Decl>;
+def TopLevelStmt : DeclNode<Decl>;
 def AccessSpec : DeclNode<Decl>;
----------------
aaron.ballman wrote:
> 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.
Indeed, I'd put the `pseudo` to qualify the decl, like `TopLevelStmtPseudoDecl` but I still don't think either is an improvement to what we have currently.


================
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;
   }
----------------
aaron.ballman wrote:
> 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.)
I wanted to do this is a separate commit. I am worried of breaking downstream users. I remember long time ago @akyrtzi was using this logic. 

There are also a bunch of tests in clang and lldb.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6358
+  case Decl::TopLevelStmt: {
+    FunctionDecl *FD = cast<TopLevelStmtDecl>(D)->getOrConvertToFunction();
+    EmitTopLevelDecl(FD);
----------------
aaron.ballman wrote:
> 
I'd like to keep this as the `FunctionDecl` type makes it move obvious what `getOrConvertToFunction` returns.


================
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.
----------------
aaron.ballman wrote:
> Are you planning on doing this as part of this patch?
I'd like to do it in a following patch as the patch will mostly touch other languages and would spill 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:
> 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.
`// REQUIRES: host-supports-jit` ensures that the system supports spawning a JIT.
`// UNSUPPORTED: system-aix` means that this test is not supported on AIX.

These options were discovered through a lot of pain and suffering by a lot of reverts. I would prefer to keep `UNSUPPORTED` line in this patch, and try to undo it in a small and atomic commit later to see if that test can be executed on AIX.


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

https://reviews.llvm.org/D127284