v.g.vassilev added a comment.
I think I have addressed all comments from @aaron.ballman and @rsmith.
================
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:
> 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.
I'd prefer doing it in a separate commit. This patch is bulky and we may need
to revert it making all bots happy. That'd be probably make downstream
consumers green/red for a while and generate a some email traffic ;)
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6165
+ // Stop squashing the top-level stmts into a single function.
+ if (CurCGF && !CXXGlobalInits.back()->getName().startswith("__stmts__")) {
+ CurCGF->FinishFunction(D->getEndLoc());
----------------
rsmith wrote:
> Instead of a name comparison, can you check whether `CXXGlobalInits.back() ==
> CurCGF->CurFn`?
Awesome! Thanks!
================
Comment at: clang/lib/Parse/ParseDecl.cpp:5395
+ // FIXME: What do we do if we get something in Stmts?
+ assert(!Stmts.size() && "Unsupported multiple stmt!");
+
----------------
rsmith wrote:
> I guess this happens in some pragma situations? Can you put a real diagnostic
> in here rather than an assert?
That's what I understood from the code. Thanks for clarifying. I added a
diagnostic.
================
Comment at: clang/lib/Parse/Parser.cpp:1032-1034
+ // FIXME: Remove the incremental processing pre-condition and verify clang
+ // still can pass its test suite, which will harden
+ // `isDeclarationStatement`.
----------------
rsmith wrote:
> Have you tried this with the latest version of the patch? Can the FIXME be
> removed?
I have tried. The failures are a couple of hundred now. However, the failing
tests are due to the fact that the tests check for expected diagnostics from
ill-formed code. There the decisions if something that's a statement or a
declaration are harder and sometimes we produce unexpected diagnostics. I am
not sure if we should be fixing the test files.
I will investigate more thoroughly but I have removed the FIXME as the majority
of the failures are resolved by the current version of the patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127284/new/
https://reviews.llvm.org/D127284
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
