https://github.com/Endilll updated
https://github.com/llvm/llvm-project/pull/78898
>From b99a75a8756a7841657fc78ffbd40f780a412f2b Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov
Date: Sun, 21 Jan 2024 16:26:29 +0300
Subject: [PATCH 1/2] [clang][Sema] Add checks for validity of default ctor's
https://github.com/Endilll updated
https://github.com/llvm/llvm-project/pull/78898
>From b99a75a8756a7841657fc78ffbd40f780a412f2b Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov
Date: Sun, 21 Jan 2024 16:26:29 +0300
Subject: [PATCH 1/2] [clang][Sema] Add checks for validity of default ctor's
https://github.com/Endilll updated
https://github.com/llvm/llvm-project/pull/78898
>From b99a75a8756a7841657fc78ffbd40f780a412f2b Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov
Date: Sun, 21 Jan 2024 16:26:29 +0300
Subject: [PATCH 1/2] [clang][Sema] Add checks for validity of default ctor's
AaronBallman wrote:
> So my takeaways here are:
>
> 1. Tests that ensure we don't crash anymore are important for us.
>
> 2. `-verify` is an acceptable way to write such tests.
>
>
> Is this correct?
Yup, that's it exactly!
https://github.com/llvm/llvm-project/pull/78898
Endilll wrote:
So my takeaways here are:
1. Tests that ensure we don't crash anymore are important for us.
2. `-verify` is an acceptable way to write such tests.
Is this correct?
https://github.com/llvm/llvm-project/pull/78898
___
cfe-commits mailing
https://github.com/AaronBallman commented:
Yeah, these changes really need to come with test coverage. We allow landing
changes with no test coverage when it's an NFC change or when the testing would
require heroic efforts. Otherwise, we expect tests that are as good as you can
reasonably
@@ -14030,6 +14034,9 @@ void
Sema::DefineImplicitDefaultConstructor(SourceLocation CurrentLocation,
CXXRecordDecl *ClassDecl = Constructor->getParent();
assert(ClassDecl && "DefineImplicitDefaultConstructor - invalid
constructor");
+ if (ClassDecl->isInvalidDecl()) {
+
https://github.com/AaronBallman edited
https://github.com/llvm/llvm-project/pull/78898
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -5990,6 +5990,10 @@ void Sema::ActOnDefaultCtorInitializers(Decl *CDtorDecl)
{
if (CXXConstructorDecl *Constructor
= dyn_cast(CDtorDecl)) {
+if (CXXRecordDecl *ClassDecl = Constructor->getParent();
+!ClassDecl || ClassDecl->isInvalidDecl()) {
+
Fznamznon wrote:
> Because we
>
Endilll wrote:
> why can't we add -verify test? Yes, it will be checking errors that the patch
> didn't touch, but it is what mostly people do when adding clang tests and it
> will be +N test cases in a regular test base which not only ensure your
> change is correct, but the future ones too.
Fznamznon wrote:
> I tried my best to explain that it's not an issues of prettiness.
Well, ok, perhaps checking that there is actually no crash in clang can be
tricky. But what I meant is, since there is no good way to check exit code, why
can't we add `-verify` test? Yes, it will be checking
Endilll wrote:
> So, there is no way to consistently check on all platforms that we didn't
> crash when an error diagnostic was issued (does clang return non-zero when
> there is error diagnostic?), is that a right understanding?
Yes. On Linux and Windows `1` is returned if error diagnostic
Fznamznon wrote:
So, there is no way to consistently check on all platforms that we didn't crash
when an error diagnostic was issued (does clang return non-zero when there is
error diagnostic?), is that a right understanding?
Still, if the tests won't be "pretty" that is not an excuse to not
Endilll wrote:
> Could you elaborate a bit more on that? What is the exact problem with the
> testing infrastructure? Can we just add a separate test with the cases from
> the issues, perhaps without -verify at all?
1) I think that the most reliable way to detect a crash would be to leverage
Fznamznon wrote:
> I decided to not include tests, because our testing infrastructure is not
> ready to test that Clang doesn't crash without overspecifying the tests using
> `-verify` mode.
Could you elaborate a bit more on that? What is the exact problem with the
testing infrastructure?
Endilll wrote:
I decided to not include tests, because our testing infrastructure is not ready
to test that Clang doesn't crash without overspecifying the tests using
`-verify` mode.
https://github.com/llvm/llvm-project/pull/78898
___
cfe-commits
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Vlad Serebrennikov (Endilll)
Changes
Fixes #10518
Fixes #67914
Fixes #78388
Also addresses the second example in #49103
This patch is based on suggestion from @cor3ntin in
https://github.com/Endilll created
https://github.com/llvm/llvm-project/pull/78898
Fixes #10518
Fixes #67914
Fixes #78388
Also addresses the second example in #49103
This patch is based on suggestion from @cor3ntin in
https://github.com/llvm/llvm-project/issues/67914#issuecomment-1896011898
19 matches
Mail list logo
