[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexander-shaposhnikov added inline comments.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+    return SubstitutedAtomicExpr;
----------------
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.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178