sammccall added a comment. In D78100#1981729 <https://reviews.llvm.org/D78100#1981729>, @rsmith wrote:
> In D78100#1981620 <https://reviews.llvm.org/D78100#1981620>, @sammccall wrote: > > > Sorry to go back and forth on this, but I'm not sure whether these > > diagnostics are actually spam to be suppressed. I think @adamcz mentioned > > these today as reasonable diagnostics we're enabling. > > > > @rsmith do you have an opinion here? > > > My initial reaction was certainly that it wasn't obvious why an > initialization error would necessarily have anything to do with a destruction > error. But this line of thinking helped me: suppose we would encounter both > an (unrecoverable) initialization error *and* an error during destruction. > How likely is it that they have a common cause? My guess would be way more > than half the time. Given that we know we're on an error recovery path, and > that we've already produced an unrecoverable error during initialization, > skipping the destruction checks is probably the better choice. Even if we're > wrong, the worst case is that the programmer fixes the initialization error > and is then presented with a destruction error that they didn't see before, > which doesn't seem all that bad of an outcome. > > (Generally I don't think we need to be sure that errors would be correlated > to suppress latter errors as being follow-on diagnostics from earlier errors, > and should lean towards suppressing diagnostics in order to make the second > and subsequent diagnostic as likely as possible to be meaningful and useful.) Thanks, that's useful. The common cause was our original intuition but I wasn't sure if that was an appropriate reason to suppress. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:11998-12001 if (Result.isInvalid()) { VDecl->setInvalidDecl(); return; } ---------------- rsmith wrote: > Should we attach a `RecoveryExpr` initializer in this case? This is D78116 (which should probably handle the case above, too) ================ 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 ---------------- 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. ================ 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 ---------------- 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. 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
