hokein marked an inline comment as done.
hokein added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11985-11986
});
if (Res.isInvalid()) {
VDecl->setInvalidDecl();
} else if (Res.get() != Args[Idx]) {
----------------
rsmith wrote:
> Should we attach a `RecoveryExpr` initializer in this case?
I think so, will address in a separate patch.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:12268
/// of sanity.
void Sema::ActOnInitializerError(Decl *D) {
// Our main concern here is re-establishing invariants like "a
----------------
sammccall wrote:
> rsmith wrote:
> > Should we attach a `RecoveryExpr` initializer in this case too?
> Now we're really slipping into a different set of use-cases for
> RecoveryExpr...
> I assume we're talking about a RecoveryExpr that has no children, at least in
> the short term, as parsing failures don't return the likely subexpressions
> found. So it's a pure error marker in the form of an AST node. Quite a lot of
> ExprError()s could become these...
>
> Actually there's another virtue here: even without subexpressions, the
> RecoveryExpr can capture the token range. So VarDecl::getSourceRange() will
> include the malformed initializer, so tools can at least attribute these
> tokens to the right part of the AST.
yeah, I'm not sure how much benefit we can get from the recovery-expr in this
case, if the initializer is failed to parse, we don't have any ast nodes.
================
Comment at: clang/test/CXX/class.access/p4.cpp:1
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions
-fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions
-fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions
-fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19
-fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19
-fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19
-fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -frecovery-ast -triple %itanium_abi_triple -fcxx-exceptions
-fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -frecovery-ast -triple %itanium_abi_triple -fcxx-exceptions
-fexceptions -fsyntax-only -verify -std=c++11 %s
----------------
sammccall wrote:
> rsmith wrote:
> > I'm not really happy about improving our quality of diagnostics only under
> > `-frecovery-ast`. Do we really need that flag? The way I see it, we can
> > divide the users of Clang up into:
> >
> > * We want valid code (eg, as a compiler): if we hit an error, it doesn't
> > matter too much whether we build `RecoveryExpr`s or not, since we're on a
> > path to bailing out anyway. If `RecoveryExpr`s allow us to improve our
> > diagnostics, we should build them. (Exception: if we want valid code or to
> > get a simple pass/fail as early as possible, maybe not.)
> > * We want to accept invalid code (eg, tooling): if we hit an error, we
> > probably want to retain as much information as we reasonably can about the
> > non-erroneous parts of the program.
> >
> > So I think at least the default should be to build `RecoveryExpr`s, and
> > maybe we can remove the flag entirely?
> Agree that want to flip the default to true, and maybe eventually get rid of
> it. But:
> - we're still fighting quite a lot of new crash-on-invalids, and can't fix
> them all in one big patch. We plan to find more by rolling this out to a
> subset of google-internal IDE users (where we have good monitoring), the flag
> is needed for this.
> - we expect this to be stable for C++ long before it can be enabled for C,
> because that requires making the C codepaths safe handle (at least
> error-)dependence. So there'll be a similar testing/improvement period later.
>
> However I don't like adding -frecovery-ast to big existing tests, we lose
> coverage of today's production behavior. I think we may need to create new
> tests to show the effect of these changes, and clean them up after flipping
> the default.
yeah, it is not perfect, given that we are at intermediate stage. We plan to
enable the flag for C++ by default (we did it once, but failed with many
crashes), this means we'd eventually focus on '-frecovery-ast' only (at least
C++), it seems not too harmful to add the -frecovery-ast flag to exiting tests
at the moment. But it is not great..
Another way is to adjust existing tests to support both, but this seems
non-trivial, maybe creating new tests for '-frecovery-ast' is a best way to go.
life can be easier if the flag is turned on by default. Currently, I have to
maintain a local patch with a long tail of adjusted tests when doing
recovery-expr developments, I need to run all tests with&without -frecovery-ast
to make sure it doesn't introduce big issures.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78100/new/
https://reviews.llvm.org/D78100
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
