================
Comment at: lib/Sema/SemaOverload.cpp:8905-8918
@@ +8904,16 @@
+ // a relevant part of the alias template instantiation.
+ assert(PD.getNumArgs() >= 1 && PDRanges.size() >= 1 &&
+ PD.getArgKind(0) == DiagnosticsEngine::ak_declcontext &&
+ (PD.getNumArgs() == 1 ||
+ (PD.getArgKind(1) == DiagnosticsEngine::ak_nameddecl &&
+ PDRanges.size() >= 2)) &&
+ "an err_typename_nested_not_found_enable_if diagnostic does"
+ " not contain the expected arguments and source ranges");
+ const bool IsAlias = PDRanges.size() > 1;
+ const NamedDecl *EnableIfOrAliasDecl =
+ IsAlias ? reinterpret_cast<NamedDecl *>(PD.getRawArg(1))
+ : cast<ClassTemplateSpecializationDecl>(
+ reinterpret_cast<DeclContext *>(PD.getRawArg(0)))
+ ->getSpecializedTemplate();
+ SourceRange Range = PDRanges[IsAlias ? 1 : 0].getAsRange();
+ // If an alias template that contains an enable_if type lookup is
----------------
Richard Smith wrote:
> Stephan Tolksdorf wrote:
> > Richard Smith wrote:
> > > I don't like inspecting the source ranges on a `PartialDiagnostic` --
> > > they're not part of the numbered argument set for the diagnostic, so
> > > someone emitting the diagnostic is within their rights to change the set
> > > of ranges they provide. More generally, I'm not really a fan of digging
> > > into the implementation of the `PartialDiagnostic` to find its arguments.
> > >
> > > Instead, how about switching the diagnostic ID from
> > > err_typename_nested_not_found_enable_if to
> > > note_ovl_candidate_disabled_by_enable_if (with comments in the .td file
> > > saying that they're required to take the same arguments)?
> > >
> > > Instead of using the `Range`, below, to determine the location for the
> > > diagnostic, you could use something like
> > > `TemplDecl->getSourceRange().contains(PDiag->first) ? PDiag->first :
> > > Templated->getLocation()`.
> > In my view a `PartialDiagnostic` (or `Diagnostic`) is not just the input to
> > some formatting routine but a strongly typed container of an error id and
> > additional pieces of information about the error. So, accessing the
> > arguments to the diagnostic through a proper API (introduced by D3060) in
> > order to generate a better error message doesn't seem that objectionable to
> > me. Also, I interpreted your FIXME as a request to do exactly this.
> >
> > I used a second source range and the
> > `err_typename_nested_not_found_enable_if` id because I assumed that in
> > `Sema::CheckTypenameType` we should always generate an error that would be
> > appropriate if the error message is not suppressed by the SFINAE logic. If
> > this is not the case, it might be cleaner to always prepare the enable_if
> > diagnostic in `CheckTypenameType` and then just reemit it in
> > `DiagnoseBadDeduction` (adding the missing `TemplateArgString`).
> >
> > If you'd like this code to not fail in the hypothetical case where someone
> > generates a `err_typename_nested_not_found_enable_if` diagnostic with
> > unexpected contents, I could replace the assert with some always-on
> > fallback logic.
> >
> >
> I don't think a `PartialDiagnostic` is even notionally strongly typed (you
> can use the same diagnostic with a string or a Decl for the same argument,
> for instance, and we do that sort of thing in some cases). The approach
> you're taking is fragile in the presence of refactoring or people reusing the
> same diagnostic message in other circumstances (where they may not provide
> the same SourceRanges). I'm also not particularly enthusiastic about a
> fallback codepath -- I'd much rather have a clean way to pass the necessary
> information through. I'm not yet sure what that mechanism is, though.
>
> (Yes, the diagnostic produced in `CheckTypenameType` should be appropriate in
> the case where we're not in a SFINAE condition.)
What I meant with strongly typed is that you can access the diagnostic
arguments in a type safe way, as the types of the arguments are recorded and
can be queried. Documenting the extra arguments in the diagnostic .td file
should minimize any fragility.
If I want to pass on additional information about an error, putting it into the
diagnostic data structure still seems rather natural to me. In this case the
only alternative I can think of is to put this as an additional field into the
`TemplateDeductionInfo`, but I don't think that would be cleaner.
================
Comment at: lib/Sema/SemaTemplate.cpp:7887-7919
@@ -7844,1 +7886,35 @@
+ << Ctx << CondRange;
+ if (const Sema::ActiveTemplateInstantiation *ATI =
+ findOutermostAliasTemplateInstantiationInSFINAEContext(*this)) {
+ // For a failed enable_if lookup within an alias template instantiation
+ // we also store the source range of the instantiation and a pointer to
+ // the TypeAliasTemplateDecl in the PartialDiagnostic.
+ // This allows us is Sema::DiagnoseBadDeduction to highlight a source
+ // range that is actually *within* the declaration of the disabled
+ // overload (and not in the declaration of the alias template).
+ // We don't restrict this handling to alias templates named enable_if_t
+ // because if a failed 'typename enable_if<...>::type' lookup occurs
+ // within an alias template instantiation in a SFINAE context, it is
+ // highly likely that the alias is used similarly to std::enable_if_t.
+ auto *AliasDecl = cast<TypeAliasTemplateDecl>(ATI->Entity);
+ // Sema::CheckTemplateIdType stores the source range for the first
+ // template argument in ATI->InstantiationRange.
+ SourceRange Arg0Range = ATI->InstantiationRange;
+ auto *EnableIfII = AliasDecl->getDeclName().getAsIdentifierInfo();
+ TemplateParameterList *TPL = AliasDecl->getTemplateParameters();
+ if (Arg0Range.isInvalid() || TPL->size() == 0)
+ CondRange = ATI->PointOfInstantiation;
+ else if (TPL->size() == 1 || EnableIfII->isStr("enable_if_t"))
+ CondRange = Arg0Range;
+ else {
+ // If the alias template has more than one parameter and is not
+ // called enable_if_t, we can't be sure that the first argument is
+ // actually the one that caused the error. So instead of underlining
+ // the full argument, we only point to the beginning of it, which
+ // should avoid misunderstandings while still pointing (close) to the
+ // origin of the error.
+ CondRange = Arg0Range.getBegin();
+ }
+ D << AliasDecl << CondRange;
+ }
return QualType();
----------------
Richard Smith wrote:
> Stephan Tolksdorf wrote:
> > Richard Smith wrote:
> > > Having reflected on this a bit more, I think we should *only* do this if
> > > the context surrounding the `enable_if` is an alias template named
> > > `enable_if_t` (and we should only walk through one such alias template).
> > > If we walk too far away from the `enable_if` expression itself, it will
> > > no longer be obvious what we're complaining about.
> > If we don't walk up all alias template instantiations in the current SFINAE
> > context on the instantiation stack, the SFINAE diagnostic will only state
> > ##no type named 'type' in 'enable_if<false>'## or ##disabled by
> > 'enable_if'## without any indication which alias template instantiation
> > caused this error, since the SFINAE diagnostic doesn't contain the stack of
> > instantiated alias templates for the suppressed error. (Would it maybe be
> > an alternative to store and print this stack when we don't have a
> > straightforward enable_if case?)
> >
> > In my opinion such a diagnostic is significantly worse than a diagnostic
> > that states something like e.g. ##disabled by 'enable_if_is_callback'## or
> > ##disabled by an error during the substitution of the alias template
> > 'enable_if_is_callback'## and that points to the source location of the
> > `enable_if_is_callback` instantiation in the function declaration
> > triggering the error.
> >
> > Could you maybe give an example for a situation where you'd find the
> > proposed diagnostic confusing? Would rewording the diagnostic in that
> > situation be an alternative to falling back to the basic SFINAE diagnostic?
> >
> >
> Hmm, suppose I have:
>
> template<typename T> void f(Iterator<T> iter);
>
> with `Iterator` an alias template. If substitution fails for any reason
> within `Iterator<T>`, I don't want to be told `disabled by 'Iterator'`. I
> think in this case I actually want to produce two notes for the substitution
> failure: one indicating that the candidate was ignored due to a substitution
> failure (pointing at the candidate), and another pointing at the location
> within the alias template where we hit the problem.
>
> I think we can do this without any alias template special-casing: when we
> come to emit a delayed 'substitution failure' diagnostic, check whether the
> location of the diagnostic is within the source range of the declaration of
> the candidate. If so, do what we do today, and if not, produce two notes (one
> pointing at the candidate and the other pointing at the location where we hit
> the error.
It would be really nice if we could tell the user that the substitution failure
occurred within `Iterator<T>`. In this case there is no other possible place,
but in more complicated situations it would make the error easier to
understand, especially if the error occurred not directly within the
`Iterator<T>` declaration but inside a nested alias template use. When you work
with with an IDE that highlights the source ranges in an editor, this can be a
real improvement. (And, of course, we can use a different wording than
"disabled by xxx" for this and similar cases.)
Without any alias template special-casing the location of the original
diagnostic for an error inside an alias template instantiation will never be
within the source range of the declaration of the candidate and `enable_if_t`
uses will generate arguably worse error messages than `enable_if` uses.
http://llvm-reviews.chandlerc.com/D3061
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
