alexander-shaposhnikov added inline comments.
================ Comment at: clang/lib/Sema/SemaConcept.cpp:263 + if (SubstitutedAtomicExpr.get()->isValueDependent()) + return SubstitutedAtomicExpr; ---------------- alexander-shaposhnikov wrote: > erichkeane wrote: > > alexander-shaposhnikov wrote: > > > erichkeane wrote: > > > > alexander-shaposhnikov wrote: > > > > > erichkeane wrote: > > > > > > So this bit is concerning to me... we have been catching a ton of > > > > > > bugs in the MLTAL stuff by trying to constant evaluate an > > > > > > expression that isn't correctly constexpr, and this will defeat it. > > > > > > We shouldn't be calling this function at all on > > > > > > non-fully-instantiated expressions. What is the case that ends up > > > > > > coming through here, and should be be calling this at all? > > > > > This happens e.g. for concepts-PR54629.cpp > > > > > > > > > > ``` > > > > > Old: > > > > > FunctionDecl 0x555564d90378 > > > > > llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, > > > > > line:32:2> line:30:6 foo 'void ()' > > > > > |-RequiresExpr 0x555564d90318 <line:31:12, col:53> 'bool' > > > > > | |-ParmVarDecl 0x555564d90150 <col:21, col:24> col:24 referenced t > > > > > 'T &' > > > > > | `-NestedRequirement 0x555564d902d8 dependent > > > > > | `-BinaryOperator 0x555564d902b8 <col:38, col:50> 'bool' '<' > > > > > | |-UnaryExprOrTypeTraitExpr 0x555564d90260 <col:38, col:46> > > > > > 'unsigned long' sizeof > > > > > | | `-ParenExpr 0x555564d90240 <col:44, col:46> 'T' lvalue > > > > > | | `-DeclRefExpr 0x555564d90220 <col:45> 'T' lvalue ParmVar > > > > > 0x555564d90150 't' 'T &' non_odr_use_unevaluated > > > > > | `-ImplicitCastExpr 0x555564d902a0 <col:50> 'unsigned long' > > > > > <IntegralCast> > > > > > | `-IntegerLiteral 0x555564d90280 <col:50> 'int' 4 > > > > > `-CompoundStmt 0x555564d90f70 <line:32:1, col:2> > > > > > > > > > > while MLTAL is empty. > > > > > ``` > > > > > (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while > > > > > clang::Sema::IsOverload invokes > > > > > clang::Sema::AreConstraintExpressionsEqual) > > > > Hmm.... that seems wrong for me, but I'm not sure how. It doesn't seem > > > > right for `AreConstraintExpressionsEqual`to try to calculate the > > > > constraint satisfaction... > > > I think we get there from > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiate.cpp#L2375 > > So I still think this is an incorrect change. I don't understand why we'd > > get here without the 'final' check, but perhaps there is something missing > > elsewhere? > unfortunately, I don't have enough context - will revisit this change (iirc > one test was failing without it and Clang was calling > CheckConstraintSatisfaction from an unexpected place). There is some > accumulated technical debt (without the current diff) / it takes to untangle. > reduced test case (extracted from concepts-PR54629.cpp): ``` template <class T> void foo() requires requires(T &t) { requires sizeof(t) < 4; } {} template <class T> void foo() requires requires(T &t) { requires sizeof(t) > 4; } {} void test() { foo<char[4]>(); } ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146178/new/ https://reviews.llvm.org/D146178 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
