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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: efriedma, erichkeane.
aaron.ballman added a comment.

In general, this looks like an improvement over the last iteration (thank you for the suggestions, @rsmith!). I think this LGTM but there's enough changes in CodeGen that I'd appreciate a second set of eyes on those changes before giving the go-ahead. I've added a few more reviewers to hopefully help with that.



================
Comment at: clang/include/clang/AST/Decl.h:4264-4267
+  Stmt *Statement = nullptr;
+  unsigned NumStmts = 0;
+  FunctionDecl *FD = nullptr;
+
----------------
Both are now unused.


================
Comment at: clang/include/clang/Basic/DeclNodes.td:98
 def FileScopeAsm : DeclNode<Decl>;
+def TopLevelStmt : DeclNode<Decl>;
 def AccessSpec : DeclNode<Decl>;
----------------
v.g.vassilev wrote:
> 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.
Yeah, I'm "okay" with the name as-is.


================
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;
   }
----------------
v.g.vassilev wrote:
> 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.
> I wanted to do this is a separate commit. I am worried of breaking downstream users. 

Downstream users have no expectation of this interface remaining stable to begin with, so I'd rather we remove the code unless someone speaks up with a concrete technical problem. That said, I'm fine doing it in a separate commit so that it's easier to raise awareness for downstreams if you think this will be disruptive to them.


================
Comment at: clang/lib/AST/Decl.cpp:5249-5250
+
+  auto *TLSD = new (C, DC) TopLevelStmtDecl(DC, BeginLoc);
+  TLSD->Statement = Statement;
+  return TLSD;
----------------
Might as well make this form of the constructor take a `Stmt *` instead of doing a two-step initialization.


================
Comment at: clang/lib/AST/DeclPrinter.cpp:936-938
+void DeclPrinter::VisitTopLevelStmtDecl(TopLevelStmtDecl *D) {
+  D->getStmt()->printPretty(Out, nullptr, Policy, Indentation, "\n", &Context);
+}
----------------
`getStmt()` can return null; should this assert we're not calling it in such a case?


================
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:
> > 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.
SGTM, thank you for investigating!


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

https://reviews.llvm.org/D127284