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