tahonermann added a comment.
The refactoring looks like a good improvement to me. I think there may be some
opportunities to make it more robust. I did some spot checking that previous
behavior is retained, but didn't exhaustively do such checking; it does look
like many of the changes were mechanical bus as Shafik noted, it is a bit
difficult to review.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:510
- /*Pattern*/ nullptr,
- /*LookBeyondLambda*/ true);
if (SetupConstraintScope(FD, TemplateArgs, MLTAL, Scope))
----------------
This would previously have passed `true` for `LookBeyondLambda` and `false` for
`IncludeContainingStruct`. The merge of both parameters to
`ForConstraintInstantiation` makes that no longer possible. I don't know if
that is a good thing or not. Perhaps this was a latent bug?
================
Comment at: clang/lib/Sema/SemaConcept.cpp:1067
- /*Pattern*/ nullptr,
- /*LookBeyondLambda*/ true);
----------------
This would previously have passed `true` for `LookBeyondLambda` and `false` for
`IncludeContainingStruct`. The merge of both parameters to
`ForConstraintInstantiation` makes that no longer possible. I don't know if
that is a good thing or not. Perhaps this was a latent bug?
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2881
- /*RelativeToPrimary*/ true, /*Pattern*/
- nullptr, /*LookBeyondLambda*/ true);
----------------
This would previously have passed `true` for `LookBeyondLambda` and `false` for
`IncludeContainingStruct`. The merge of both parameters to
`ForConstraintInstantiation` makes that no longer possible. I don't know if
that is a good thing or not. Perhaps this was a latent bug?
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:48
+ bool IsDone = false;
+ bool RelativeToPrimaryOff = true;
+ static Response Done() {
----------------
Suggestion: rename `RelativeToPrimaryOff` to `ClearRelativeToPrimary` or
`SetRelativeToPrimaryFalse`.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:231
///
/// \param D the declaration for which we are computing template instantiation
/// arguments.
----------------
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:234
///
/// \param Innermost if non-NULL, the innermost template argument list.
///
----------------
Tangent: When I was investigating this function a while back, I was uncertain
what purpose `Innermost` serves and I didn't find the comment helpful. It
appears that there is exactly one call to `getTemplateInstantiationArgs()` that
passes a non-null value for it. That occurs in
`Sema::CheckTemplateArgumentList()` and happens because a template is passed as
`ND` and, while template arguments have been identified, a specialization has
not yet been formed, so those arguments are passed for `Innermost`.
I suggest updating the comment:
/// \param Innermost if non-NULL, specifies a template argument list for the
template
/// declaration passed as \p ND.
Perhaps it is worth adding an assert that, if `Innermost` is non-null, that
`ND` corresponds to a template declaration (not a specialization).
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:242
/// \param Pattern If non-NULL, indicates the pattern from which we will be
/// instantiating the definition of the given declaration, \p D. This is
/// used to determine the proper set of template instantiation arguments for
----------------
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:286
}
- bool IsFriend = Rec->getFriendObjectKind() ||
- (Rec->getDescribedClassTemplate() &&
-
Rec->getDescribedClassTemplate()->getFriendObjectKind());
- if (IncludeContainingStructArgs && IsFriend &&
- Rec->getNonTransparentDeclContext()->isFileContext() &&
- (!Pattern || !Pattern->getLexicalDeclContext()->isFileContext())) {
- Ctx = Rec->getLexicalDeclContext();
- RelativeToPrimary = false;
- continue;
- }
}
----------------
================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:292-295
+ if (R.NextDecl)
+ CurDecl = R.NextDecl;
+ else
+ CurDecl = Decl::castFromDeclContext(CurDecl->getDeclContext());
----------------
This seems a little fragile. How about rolling the `else` case into the main
if/else chain above so that `R.NextDecl` is always set? We could then assert on
it. I added an illustrative suggested edit above. This would require changing a
number of the handlers to set `NextDecl` where they currently don't, but being
explicit about the decl navigation in those cases might make the flow easier to
understand.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134874/new/
https://reviews.llvm.org/D134874
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
