alexander-shaposhnikov added inline comments.
================ Comment at: clang/lib/Sema/SemaConcept.cpp:263 + if (SubstitutedAtomicExpr.get()->isValueDependent()) + return SubstitutedAtomicExpr; ---------------- alexander-shaposhnikov wrote: > 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]>(); > } > ``` so I've investigated a bit how things work right now (without this diff), the following code is somewhat (but not directly) relevant: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaConcept.cpp#L417 (from https://github.com/llvm/llvm-project/commit/b25902736c2e330429278e1929cc5afd2201fb77) however, previously, the issues above (for the previous version of this diff) (e.g. for concepts-PR54629.cpp) were triggered when Clang was trying to substitute an empty MLTAL, while in our case it's unnecessary. It might happen though that there is another problem lingering around but it looks like at the moment the tests are passing without changes to ``` calculateConstraintSatisfaction ``` (=> dropped them) 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
