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

[Erich Keane via Phabricator via cfe-commits]

erichkeane 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:
> > > > 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?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:227
+        (TSTy = Ty->getAs<TemplateSpecializationType>()))
+      Result.addOuterTemplateArguments(const_cast<FunctionTemplateDecl *>(FTD),
+                                       TSTy->template_arguments(),
----------------
alexander-shaposhnikov wrote:
> erichkeane wrote:
> > So I'd come up with something similar, but this ends up being a little goofy?  And I thought it didn't really work in 1 of the cases.  I wonder if we're better off looking at the decl contexts to try to figure out WHICH of the cases we are, and just set the next decl context based on that?
> sure, we can try. In the meantime - regarding GH62110 - do you happen to have any thoughts on that ?
There is nothing that sticks out from reading it.  I'm unfortunately without my work machine for Friday/today, so I haven't had time to spend in the debugger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178