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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
