[PATCH] D137712: Correctly handle Substitution failure in concept specialization.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:103
 
+  bool hasSubstitutionFailureInArgs() const {
+    return ArgsHasSubstitutionFailure;
----------------
erichkeane wrote:
> Does this really belong here instead of as a part of the ConceptSpecializationDecl?
I am not aware of the original goals for this. For example, `ASTTemplateArgumentListInfo` is part of `ConceptReference` while `TemplateArgument` is part of `ImplicitConceptSpecializationDecl`. Both of them are invalid in such a case.
I am open to suggestion to where to place this.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:159
+  // todo: add doc ?
+struct SubstitutionDiagnostic {
+  StringRef SubstitutedEntity;
----------------
erichkeane wrote:
> I had a previous patch at something else where I was moving toward doing this change, so I think this is probably something inevitable.  
> 
> However, I'm having a tough time splitting this patch mentally between the 'fix' and the infrastructure changes needed.  A part of me thinks we should split this patch a bit in that direction.
I agree. This is a larger change which probably belongs to a separate change. Until then I think it is fine to not provide diagnostic of the exact expression of the SubstitutedEntity for which SF occurred.


================
Comment at: clang/lib/AST/ExprConcepts.cpp:112-113
+  return ConceptSpecializationExpr::Create(
+      C, NNS, TemplateKWLoc, ConceptNameInfo, FoundDecl, NamedConcept, nullptr,
+      nullptr, Satisfaction);
+}
----------------
shafik wrote:
> I think I got the parameter names correct.
Could you please elaborate what is your suggestion here ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137712