[PATCH] D149612: [Sema] avoid merge error type

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGef107afd48a9: [Sema] avoid merge error type (authored by HerrCai0907). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 524001. HerrCai0907 added a comment. simplify test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

hokein accepted this revision. hokein added a comment. thanks, looks good to me. Comment at: clang/test/Sema/merge-decls.c:96 +char d; +char x[sizeof(d.data) == 8]; // expected-error {{member reference base type 'char' is not a structure or union}} +char x[sizeof(d.data) ==

[PATCH] D149612: [Sema] avoid merge error type

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I think this is looking fine to me. Thanks for your patience on this, I know it was a tough review. Comment at: clang/lib/Sema/SemaDecl.cpp:4399

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 523870. HerrCai0907 added a comment. revert untill first version Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

hokein added a comment. In D149612#4346068 , @sammccall wrote: > Unless I'm wildly misremembering (@hokein knows better), the type should also > be dependent in C. > > The intuition is that the code has some meaning that depends on how the error > is

[PATCH] D149612: [Sema] avoid merge error type

sammccall added a comment. Unless I'm wildly misremembering (@hokein knows better), the type should also be dependent in C. The intuition is that the code has some meaning that depends on how the error is resolved (by the programmer changing the code!), and that "dependent" is a good

[PATCH] D149612: [Sema] avoid merge error type

aaron.ballman added subscribers: sammccall, hokein. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2582 } else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) { -T = Context.getDependentSizedArrayType(T, ArraySize, ASM,

[PATCH] D149612: [Sema] avoid merge error type

erichkeane added a comment. In D149612#4343440 , @HerrCai0907 wrote: > Maybe return `QualType()` as a temporary solution to avoid crash? @erichkeane I'm not sure at the moment... I'd like to have aaron take a look and chat it over with him.

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added a comment. Maybe return `QualType()` as a temporary solution to avoid crash? @erichkeane Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 ___

[PATCH] D149612: [Sema] avoid merge error type

erichkeane added a comment. @aaron.ballman : Can you comment, particularly on the 'x2' and 'x3' examples here? I think our hackery here to get the AST back in a reasonable position here is unfortunate, and leads to some pretty awkward errors. I'm not sure what else we could do? We can't

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2583 +if (ArraySize->containsErrors()) { + RecoveryExpr *RE = RecoveryExpr::Create( + Context, ArraySize->getType(), ArraySize->getBeginLoc(), erichkeane wrote: >

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 520808. HerrCai0907 added a comment. add more test cases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 520807. HerrCai0907 added a comment. add more test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2583 +if (ArraySize->containsErrors()) { + RecoveryExpr *RE = RecoveryExpr::Create( + Context, ArraySize->getType(), ArraySize->getBeginLoc(), HerrCai0907 wrote: >

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 520496. HerrCai0907 marked 2 inline comments as done. HerrCai0907 added a comment. use 1 replace 0 as length Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2583 +if (ArraySize->containsErrors()) { + RecoveryExpr *RE = RecoveryExpr::Create( + Context, ArraySize->getType(), ArraySize->getBeginLoc(), erichkeane wrote: >

[PATCH] D149612: [Sema] avoid merge error type

erichkeane requested changes to this revision. erichkeane added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Sema/SemaType.cpp:2583 +if (ArraySize->containsErrors()) { + RecoveryExpr *RE = RecoveryExpr::Create( +

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added a comment. > So I'm still not sure this is the 'right' away about it. I think we should > instead properly handle the containsErrors case and just always create a > non-dependent sized array, except with the RecoveryExpr having the correct > type. You are right. Fix as your

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 519972. HerrCai0907 added a comment. update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaType.cpp

[PATCH] D149612: [Sema] avoid merge error type

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2582 } else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) { -T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets); +if (getLangOpts().CPlusPlus) { +

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2582 } else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) { -T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets); +if (getLangOpts().CPlusPlus) { +

[PATCH] D149612: [Sema] avoid merge error type

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2582 } else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) { -T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets); +if (getLangOpts().CPlusPlus) { +

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 519675. HerrCai0907 added a comment. use nullpt as SizeExpr Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 519671. HerrCai0907 added a comment. getConstantArrayType instal QualType() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added a comment. Dependent type for CPP is a common case due to template. But for `mergeTypes` in C, dependent type cannot be handled correctly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/

[PATCH] D149612: [Sema] avoid merge error type

erichkeane added a comment. In D149612#4314536 , @shafik wrote: > In D149612#4314209 , @HerrCai0907 > wrote: > >> in SemaType.cpp#BuildArrayType, It will generate `DependentSizedArrayType` >> which cannot be

[PATCH] D149612: [Sema] avoid merge error type

shafik added a subscriber: rsmith. shafik added a comment. In D149612#4314209 , @HerrCai0907 wrote: > in SemaType.cpp#BuildArrayType, It will generate `DependentSizedArrayType` > which cannot be merged. > So we can either add additional check in

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 updated this revision to Diff 518915. HerrCai0907 added a comment. fix in `Sema::BuildArrayType` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149612/new/ https://reviews.llvm.org/D149612 Files: clang/docs/ReleaseNotes.rst

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 added a comment. in SemaType.cpp#BuildArrayType, It will generate `DependentSizedArrayType` which cannot be merged. So we can either add additional check in `MergeVarDeclTypes` or directly ignore to generate this type in `BuildArrayType`. Which one is better? } else if

[PATCH] D149612: [Sema] avoid merge error type

shafik added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:4395 bool MergeTypeWithOld) { - if (New->isInvalidDecl() || Old->isInvalidDecl()) + if (New->isInvalidDecl() || Old->isInvalidDecl() || New->getType()->containsErrors() ||

[PATCH] D149612: [Sema] avoid merge error type

HerrCai0907 created this revision. HerrCai0907 added reviewers: aaron.ballman, rjmccall, shafik. Herald added a project: All. HerrCai0907 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. fixed: