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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to