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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
