[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;
----------------
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?


================
Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa<CXXRecordDecl>(ND) && isa<CXXRecordDecl>(OtherND))
+    ForConstraintInstantiation = true;
----------------
Hmm... this seems really strange to have to do. `ForConstraintInstantiation` shouldn't be used here, the point of that is to make sure we 'keep looking upward' once we hit a spot we normally stop with.  What exactly is the issue that you end up running into here?  Perhaps I can spend some time debugging what we should really be doign.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:220
+Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD) {
+  return Response::ChangeDecl(FTD->getLexicalDeclContext());
+}
----------------
Huh, glad this ends up being useful!  I think i suggested this at one point in the last version of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178