[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik added a comment. In D83008#2136303 , @teemperor wrote: > I think we are talking about different things. My question was why is this a > 'Shell' test (like, a test in the `Shell` directory that uses FileCheck) and > not a test in the `API` director

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik updated this revision to Diff 276229. shafik marked 5 inline comments as done. shafik added a comment. - Removing spurious local variables in test - Simplifying the test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83008/new/ https://reviews.llvm.org/D83008 Files: clang/lib/A

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2197 // least as many of them as possible). return RD->isTrivial() && RD->isCXX11StandardLayout(); } teemperor wrote: > See here for the POD check that we might get wrong

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik updated this revision to Diff 276407. shafik added a comment. Moved from shell test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83008/new/ https://reviews.llvm.org/D83008 Files: clang/lib/AST/RecordLayoutBuilder.cpp lldb/test/API/lang/cpp/alignas_base_class/Makefile lldb

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

This revision was automatically updated to reflect the committed changes. Closed by commit rG63b0f8c788d8: [RecordLayout] Fix ItaniumRecordLayoutBuilder so that is grabs the correct… (authored by shafik). Herald added projects: clang, LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 258084. shafik added a comment. - Added unit test - Modified condition for defining `FromRecord` to whether it has an `ExternalSource` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 Files: clang/lib/AST/

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8173 + // If a RecordDecl has base classes they won't be imported and we will + // be missing anything that we inherit from those bases. + if (FromRecor

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 259110. shafik marked 2 inline comments as done. shafik added a comment. - Simplifying the changes in `ASTImporter::ImportContext` - Removing `DC->setHasExternalLexicalStorage(true);` from unit test since we are checking `getExternalSource()` CHANGES SINCE L

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 259703. shafik marked 2 inline comments as done. shafik added a comment. Capitalization fixes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 Files: clang/lib/AST/ASTImporter.cpp clang/unittests/AST/ASTI

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

This revision was automatically updated to reflect the committed changes. Closed by commit rGdef7c7f60205: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...) (authored by shafik). Herald added projects: clang, LLDB. Changed prior to commit: https://reviews.llvm.org/D7800

[PATCH] D132695: [Clang] Avoid crashes when parsing using enum declarations

shafik updated this revision to Diff 456128. shafik marked an inline comment as done. shafik added a comment. - Move `DS.SetTypeSpecError()` outside the `if` so that it is always called when the diagnostic is triggered. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132695/new/ https://

[PATCH] D132695: [Clang] Avoid crashes when parsing using enum declarations

This revision was automatically updated to reflect the committed changes. Closed by commit rGaa7ce60536a6: [Clang] Avoid crashes when parsing using enum declarations (authored by shafik). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D132831: [clang][Interp] Handle SubstNonTypeTemplateParmExprs

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. Makes sense to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132831/new/ https://reviews.llvm.org/D132831 _

[PATCH] D132832: [clang][Interp] Handle missing local initializers better

shafik accepted this revision. shafik added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132832/new/ https://reviews.llvm.org/D132832 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mail

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

shafik added subscribers: aprantl, shafik. shafik added a comment. Maybe @aprantl you want to take a look at this. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1761 - if (Method->isVirtual()) { + if (Method->isVirtual() && !Method->isConsteval()) { if (Method->isPure(

[PATCH] D132906: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is a special member function before attempting to call DefineDefaultedFunction(...)

shafik created this revision. shafik added reviewers: erichkeane, aaron.ballman. Herald added a project: All. shafik requested review of this revision. In `Sema::CheckCompletedCXXClass(...)` It used a lambda `CheckForDefaultedFunction` the `CXXMethodDecl` passed to `CheckForDefaultedFunction` ma

[PATCH] D132918: [clang] Fix a crash in constant evaluation

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4797 } +// Can't access properties of an incomplete type. +if (!RD->hasDefinition()) { erichkeane wrote: > It seems to me that we shouldn't GET to this function with an incomple

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1761 - if (Method->isVirtual()) { + if (Method->isVirtual() && !Method->isConsteval()) { if (Method->isPure()) ---

[PATCH] D132906: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is a special member function before attempting to call DefineDefaultedFunction(...)

shafik updated this revision to Diff 456827. shafik added a comment. - Fixed formatting in test - Added release note CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132906/new/ https://reviews.llvm.org/D132906 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaDeclCXX.cpp clang/

[PATCH] D132906: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is a special member function before attempting to call DefineDefaultedFunction(...)

shafik added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6957 // FIXME: We can defer doing this until the vtable is marked as used. -if (M->isDefaulted() && M->isConstexpr() && M->size_overridden_methods()) +if (CSM != CXXInvalid && M->isDefaulted() &

[PATCH] D132906: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is a special member function before attempting to call DefineDefaultedFunction(...)

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb9f767884669: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the… (authored by shafik). Herald added a project: clang. Reposito

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane, rsmith. Herald added a project: All. shafik requested review of this revision. Based on the changes introduced by 15361a21e01026e74cb17011b702c7d1c881ae94 it looks like C++17 compatibility diagnostic should have been

[PATCH] D132997: [clang][Interp] Handle DeclRefExpr of reference types

shafik added a comment. I think like @aaron.ballman was saying in another PR you should aim for as complete set of tests as possible even if you have to comment one that can't work yet. For example cases that would involve `MemberExpr` or `UsingShadow` for example as well as the cases you menti

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

shafik added a comment. Do you have an idea of how common this might be? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133029/new/ https://reviews.llvm.org/D133029 ___ cfe-commits mailing list cfe-commit

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:1534-1538 + if (TInfo->getType()->getContainedAutoType()) { Diag(D.getIdentifierLoc(), diag::warn_cxx14_compat_template_nontype_parm_auto_type) << QualType(TInfo->getType()->getContai

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:1534-1538 + if (TInfo->getType()->getContainedAutoType()) { Diag(D.getIdentifierLoc(), diag::warn_cxx14_compat_template_nontype_parm_auto_type) << QualType(TInfo->getType()->getContai

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik updated this revision to Diff 457471. shafik added a comment. - Updated to check contained deduced type before checking if it is an `AutoType` - Split out test into C++20 and C++17 parts CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132990/new/ https://reviews.llvm.org/D132990 F

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp:7 + +template // ok, no diagnostic expected +void func() {} mizvekov wrote: > I think this should have a new diagnost

[PATCH] D133177: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is not deleted before attempting to call DefineDefaultedFunction(...)

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane. Herald added a project: All. shafik requested review of this revision. I discovered this additional bug at the end of working on D132906 In `Sema::CheckCompletedCXXClass(...) ` use

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

shafik added a comment. In D133029#3763688 , @ilya-biryukov wrote: > In D133029#3763120 , @shafik wrote: > >> Do you have an idea of how common this might be? > > I don't have the numbers yet, but this broke the

[PATCH] D133194: rewording note note_constexpr_invalid_cast

shafik added a comment. I can't find any C tests that exercise this diagnostic. We should add a C test as well to confirm that we obtain the diagnostic we expect in a C context. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133194/new/ https://rev

[PATCH] D133177: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is not deleted before attempting to call DefineDefaultedFunction(...)

shafik updated this revision to Diff 457736. shafik added a comment. - Add Release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133177/new/ https://reviews.llvm.org/D133177 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaDeclCXX.cpp clang/test/SemaCXX/constant-expres

[PATCH] D133177: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the CXXMethodDecl is not deleted before attempting to call DefineDefaultedFunction(...)

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9f6b3199d33c: [Clang] Fix lambda CheckForDefaultedFunction(...) so that it checks the… (authored by shafik). Herald added a project: clang. Reposito

[PATCH] D133289: [C2X] N3007 Type inference for object definitions

shafik added a comment. The proposal has several nice tests and I don't they are all covered here e.g.: { double a = 7; double b = 9; { double b = b * b; // undefined, uses uninitialized variable without address Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D132918: [clang] Fix a crash in constant evaluation

shafik added a comment. In D132918#3766909 , @ilya-biryukov wrote: > I was just passing by, but wanted to add more context from our investigation > with @kadircet. > If variables with incomplete types appear inside non-template `constexpr` > function t

[PATCH] D132918: [clang] Fix a crash in constant evaluation

shafik accepted this revision. shafik added a comment. LGTM, I spent some time debugging this and I don't see an obvious alternative. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132918/new/ https://reviews.llvm.org/D132918 __

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

shafik added a comment. First set of comments. Comment at: clang/include/clang/Parse/Parser.h:2418 +DS, TemplateInfo, AS, DSC, LateAttrs, +(ImplicitTypenameContext)isImplicitTypenameContext(DSC)); + } Why don't we just have `isImplicitTypenameC

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

shafik added a comment. Maybe I missed it but I don't see a test that covers temp.res p5 e.g.: template void f(int i) { T::x * i; // This will be assumed to be the expression // T::x multiplied by i // Not a dec

[PATCH] D133499: [clang]: Add DeclContext::dumpDecl() in order to conveniently dump an AST from a DeclContext.

shafik added a comment. Nice, thank you for doing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133499/new/ https://reviews.llvm.org/D133499 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D133499: [clang]: Add DeclContext::dumpDecl() in order to conveniently dump an AST from a DeclContext.

shafik added inline comments. Comment at: clang/lib/AST/ASTDumper.cpp:203 +LLVM_DUMP_METHOD void DeclContext::dumpDecl() const { + if (const Decl *D = dyn_cast(this)) Interesting ` DeclContext::dumpDeclContext()` is in `DeclPrinter.cpp` I wonder why these wer

[PATCH] D133522: [clang] Fix Complex x Float x Int conversions so it handles type sugar

shafik added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1135 +QualType RHSElementType = +RHSComplexType ? RHSComplexType->getElementType() : RHSType; +QualType ResultType = Can you collapse this check with the next line? It just look

[PATCH] D133499: [clang]: Add DeclContext::dumpDecl() in order to conveniently dump an AST from a DeclContext.

shafik added inline comments. Comment at: clang/lib/AST/ASTDumper.cpp:205 + if (const Decl *D = dyn_cast(this)) +D->dump(); +} aaron.ballman wrote: > shafik wrote: > > aaron.ballman wrote: > > > erichkeane wrote: > > > > aaron.ballman wrote: > > > > > erichk

[PATCH] D133289: [C2X] N3007 Type inference for object definitions

shafik added inline comments. Comment at: clang/test/Sema/c2x-auto.c:49 +auto b = 9; +auto c = a + b; + } When I made the comment about the example from the proposal, this was what I was thinking about. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D133574: [C2x] reject type definitions in offsetof

shafik added inline comments. Comment at: clang/include/clang/Parse/Parser.h:2283 case DeclSpecContext::DSC_top_level: +case DeclSpecContext::DSC_offsetof: return true; Why `true` for this case? What does this allow that we want? Do we test it?

[PATCH] D133648: Clang, increase upper bound of partially initialized array sizes

shafik added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:866 ElementEntity.getKind() == InitializedEntity::EK_VectorElement) ElementEntity.setElementIndex(Init); `setElementIndex(...)` takes `unsigned` as well and therefore `Initia

[PATCH] D133659: [Clang] P1169R4: static operator()

shafik added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15934 + // or a reference to an enumeration. + // Note: Before C++23, a member function also has to not be static. if (CXXMethodDecl *MethodDecl = dyn_cast(FnDecl)) { =

[PATCH] D133807: Update Unicode to 15.0

shafik added a comment. Thank you for doing this work. Comment at: llvm/lib/Support/UnicodeCaseFold.cpp:713 + // 8 characters + if (C <= 0xa7c2) return C | 1; Maybe I am misunderstanding the comments but should this be `0xa7be`? Commen

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik updated this revision to Diff 460229. shafik marked 6 inline comments as done. shafik added a comment. - Updating testing to reflect comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132990/new/ https://reviews.llvm.org/D132990 Files: clang/lib/Sema/SemaTemplate.cpp cl

[PATCH] D132990: [Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

shafik added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:1534-1538 + if (const auto *T = TInfo->getType()->getContainedDeducedType()) +if (isa(T)) + Diag(D.getIdentifierLoc(), + diag::warn_cxx14_compat_template_nontype_parm_auto_type) +

[PATCH] D133887: [Clang] Support label at end of compound statement

shafik added a comment. The proposal says: > A label at the end of a compound statement is treated as if it were followed > by a null statement Does it make sense to write an AST test to verify this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

shafik added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290 +return this->emitConst( +E, Ctx.getASTContext().getTypeSizeInChars(ArgType).getQuantity()); + } I notice that `HandleSizeof` special cases `void` and function typ

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping @dim @rsmith @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

shafik added a comment. ping @dim @rsmith @aaron.ballman CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148474/new/ https://reviews.llvm.org/D148474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman

[PATCH] D128449: [clang] Introduce -Warray-parameter

shafik added inline comments. Comment at: clang/test/Sema/array-parameter.c:17 + +void f5(int a[restrict 2]); +void f5(int a[2]); // no warning Since we are covering `static`, `const` and `restict` we should also cover `volatile` for completeness. CHANGES SIN

[PATCH] D128248: [clang] Avoid an assertion in APValue::hasArrayFiller()

shafik added a comment. Are you sure it is failing on the line you indicated? After looking at the code, a little before that line we have: Result = APValue(APValue::UninitArray(), NumEltsToInit, NumElts); which should assure that we have an array at this point. Looking at the result of the

[PATCH] D122768: [Clang][C++20] Support capturing structured bindings in lambdas

shafik added a comment. I think I need to do another sweep of this PR but mostly minor comments. You should add tests for structured bindings binding to array and tuple-like types b/c the AST under the `BindingDecl` will be different for each of those cases and it would be worth verifying it wo

[PATCH] D128747: ISSUE - incorrect -Winfinite-recursion warning on potentially-unevaluated operand #21668

shafik added a comment. Thank for looking at this bug, just a small comment on the test. Comment at: clang/test/SemaCXX/warn-infinite-recursion.cpp:178 +struct Q { + virtual ~Q(){}; +}; Looks like your missing `// expected-warning{{call itself}}` Repository

[PATCH] D128248: [clang] Avoid an assertion in APValue::hasArrayFiller()

shafik added a comment. > and run the test case, the first added line prints `1` and the second one > `0`. `Result` is being mutated when doing the in-place evaluation. I did not catch that. That is unfortunate, I am wondering now if we need a `Result.isArray() && ` before the `EvaluateInPlace`

[PATCH] D128248: [clang] Avoid an assertion in APValue::hasArrayFiller()

shafik added inline comments. Comment at: clang/include/clang/AST/APValue.h:511-512 bool hasArrayFiller() const { +if (!isArray()) + return false; return getArrayInitializedElts() != getArraySize(); aaron.ballman wrote: > I think this makes the i

[PATCH] D128248: [clang] Avoid an assertion in APValue::hasArrayFiller()

shafik accepted this revision. shafik added a comment. LGTM after addressing all of Aaron's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128248/new/ https://reviews.llvm.org/D128248 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D128119: [clang] enforce instantiation of constexpr template functions

shafik added a comment. During the parsing of the assignment in `main` for the `constexpr` case we end up in `Sema::MarkFunctionReferenced` in particular this code: else if (Func->isConstexpr()) // Do not defer instantiations of constexpr functions, to avoid the // expression evalu

[PATCH] D129277: [clang] [Serialization] Fix swapped PPOpts/ExistingPPOpts parameters. NFC.

shafik added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:5174 std::string &SuggestedPredefines) override { - return checkPreprocessorOptions(ExistingPPOpts, PPOpts, nullptr, FileMgr, + return checkPreprocessorOpti

[PATCH] D128119: [clang] Enforce instantiation of constexpr template functions during non-constexpr evaluation

shafik accepted this revision. shafik added a comment. LGTM, thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128119/new/ https://reviews.llvm.org/D128119 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D129373: [NFC] Minor cleanup of usage of FloatModeKind with bitmask enums

shafik added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:894 bool useObjCFPRetForRealType(FloatModeKind T) const { -return RealTypeUsesObjCFPRetMask & llvm::BitmaskEnumDetail::Underlying(T); +return (int)((FloatModeKind)RealTypeUsesObjCFPRetMask

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik updated this revision to Diff 447785. shafik marked 3 inline comments as done. shafik added a comment. - Added documentation for `getValueRange(...)` - Added new tests to cover fix introduced by D130301 CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik updated this revision to Diff 447825. shafik marked an inline comment as done. shafik added a comment. - Changed wording for diagnostic and updated the places where it was used and checked in tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130058/new/ https://reviews.llvm.or

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added inline comments. Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2420 + constexpr E1 x2 = static_cast(8); // expected-error {{must be initialized by a constant expression}} + // expected-note@-1 {{integer value 8 is outside the valid range of values

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik updated this revision to Diff 448173. shafik marked 14 inline comments as done. shafik added a comment. - Add release note - Reflow diagnostic - Update defect report status - Add test for E3 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130058/new/ https://reviews.llvm.org/D13005

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa3710589f285: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum… (authored by shafik). Herald added a project: clang. Re

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik updated this revision to Diff 448390. shafik added a comment. - dyn_cast was not seeing through type sugar switch to `isEnumeralType()` - `APInt` requires the bit widths to be the same when comparing and so switched to obtaining the value from `Result` from just `getInt()` to `getInt().ge

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added inline comments. Comment at: clang/test/Analysis/cfg.cpp:227 +enum MyEnum : int { A, B, C }; static const enum MyEnum D = (enum MyEnum) 32; This is undefined behavior unless the enum has a fixed type, using `: int` above does that.

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. In D130058#3688049 , @thakis wrote: > Here's a reduced repro of one case: > > % cat test.cc > typedef enum VkResult { > VK_ERROR_INVALID_OPAQUE_CAPTURE_ADDRESS = -1000257000, > VK_RESULT_MAX_ENUM = 0x7FFF > }

[PATCH] D130811: [Clang] Fix handling of Max from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...)

shafik created this revision. shafik added reviewers: erichkeane, aaron.ballman, tahonermann, thakis. Herald added a project: All. shafik requested review of this revision. This is a follow-up to D130058 to fix how we handle the `Max` value we obtain from `getVa

[PATCH] D130811: [Clang] Fix handling of Max from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...)

shafik added a comment. @thakis I created a PR for the fix to D130058 since I wanted to document this. As long the pre-commit checks don't show anything off I will land this. I added a test to also cover your case as well. CHANGES SINCE LAST ACTION https:/

[PATCH] D130811: [Clang] Fix handling of Max from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...)

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa0d610516288: [Clang] Fix handling of Max from getValueRange(...)

[PATCH] D130936: [SemaCXX] Validate destructor is valid for dependent classes

shafik added a comment. LGTM but let's have Erich take a look. Comment at: clang/lib/Sema/SemaDecl.cpp:11499 - // FIXME: Shouldn't we be able to perform this check even when the class - // type is dependent? Both gcc and edg can handle that. The FIX

[PATCH] D130811: [Clang] Fix handling of Max from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...)

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:13540 ED->getValueRange(Max, Min); +--Max; erichkeane wrote: > I don't think this is the correct answer. Even though the other use of this > seems to 'work', `getValueRa

[PATCH] D130811: [Clang] Fix handling of Max from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...)

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:13540 ED->getValueRange(Max, Min); +--Max; erichkeane wrote: > shafik wrote: > > erichkeane wrote: > > > I don't think this is the correct answer. Even though the other u

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. > Given that we have a non-obvious (at least to me) issue in a widely used > third-party library, would we consider giving users some way to opt out of > this error, at least as a transition tool? Thank your feedback, this is very helpful. I won't have time to dig into t

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. In D130058#3701485 , @smeenai wrote: > In D130058#3698213 , @shafik wrote: > >>> Given that we have a non-obvious (at least to me) issue in a widely used >>> third-party library, would we

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane, thakis, smeenai, tahonermann. Herald added a project: All. shafik requested review of this revision. In D130058 we diagnose the undefined behavior of setting the value outside the r

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:13532 -if (DestType->isEnumeralType()) { +if (Info.Ctx.getLangOpts().CPlusPlus && DestType->isEnumeralType()) { const EnumType *ET = dyn_cast(DestType.getCanonicalType()); C

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcc104113ddec: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed… (authored by shafik). Herald added a project: clang. Ch

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. In D131307#3708413 , @ronlieb wrote: > hi, latest commit seems to have broken buildbot for amdgpu > https://lab.llvm.org/buildbot/#/builders/193/builds/16651 This looks like a real bug, the enum which has a typedef `hsa_agent_inf

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

shafik accepted this revision. shafik added a comment. LGTM as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128113/new/ https://reviews.llvm.org/D128113 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D131423: [clang] fix frontend crash when evaluating type trait

shafik added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:5307 // T t(create()...); +if (Args.empty()) + return false; I don't believe this is the right fix, the assert below is saying that we should not be here if `Args.empty()`

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

shafik added inline comments. Comment at: clang/lib/AST/APValue.cpp:658 for (auto &Val : Inits) { int64_t Char64 = Val.getInt().getExtValue(); if (!isASCII(Char64)) I believe to be fully correct we also need to add: ``` if (!Val.isInt()) return fa

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

shafik added a comment. I wonder if `APValue` needs a member function to verify the underlying type of an `Array` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131466/new/ https://reviews.llvm.org/D131466 _

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. In D131307#3709644 , @erichkeane wrote: > > That IS Undefined Behavior, but I think was unintended by this patch. The > intent of this patch (and its parent) was to diagnose this UB during > `constexpr` evaluation. HOWEVER

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

shafik created this revision. shafik added reviewers: erichkeane, aaron.ballman, zatrazz. Herald added a project: All. shafik requested review of this revision. In D131307 we allowed the diagnostic to be turned into a warning for a transition period. This had t

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. @zatrazz please see D131528 which should prevent the diagnostic for triggering for the example you showed us. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131307/new/ https://reviews.llvm

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

shafik updated this revision to Diff 451318. shafik added a comment. - Adding test to confirm the warning does not trigger outside of a constant expression context. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131528/new/ https://reviews.llvm.org/D131528 Files: clang/lib/AST/ExprCo

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

This revision was automatically updated to reflect the committed changes. Closed by commit rG4e458765aaef: [Clang] Restrict non fixed enum to a value outside the range of the enumeration… (authored by shafik). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D131479: Handle explicitly defaulted consteval special members.

shafik added inline comments. Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:812 + fail1 = good0; // expected-error-re {{call to consteval function '{{.*::copy<.*::foo>}}::operator=' is not a constant expression}} \ + expected-note {{in

[PATCH] D131423: [clang] fix frontend crash when evaluating type trait

shafik added a comment. In D131423#3712083 , @inclyc wrote: > @aaron.ballman @shafik (Help wanted). These type traits will not cause clang > to crash if current patch applied, but the `bool x` in the test case will > evaluate to `false`. I think the p

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. In D131307#3713011 , @sberg wrote: > With this commit, > > $ cat test.cc > #include "boost/numeric/conversion/cast.hpp" > int main() { return boost::numeric_cast(0L); } > > $ clang++ test.cc > > succeeds without any diag

[PATCH] D121532: [Clang][WIP] Fix Unevaluated Lambdas

shafik added a comment. Can you add a test to `ASTImpoterTest.cpp` that checks that we import the `LambdaDependencyKind` correctly? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121532/new/ https://reviews.llvm.org/D121532 ___

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik created this revision. shafik added reviewers: aprantl, dblaikie, probinson. Herald added a project: All. shafik requested review of this revision. D70524 added support for auto return types for C++ member functions. I was implementing support on the LLDB

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik added a comment. I did some experiments where I did not restrict this to lambdas case and I could not come up with a test case where we emit debug info and do not have the deduced type. Are there cases where we emit debug info and we don't have the deduced type? If not what do we gain b

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik updated this revision to Diff 422081. shafik added a comment. Adding test case CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123319/new/ https://reviews.llvm.org/D123319 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/test/CodeGenCXX/no_auto_return_lambda.cpp Index: clang/t

<    1   2   3   4   5   6   7   8   9   10   >