Thanks for the review!
================
Comment at: include/clang/Basic/SourceLocation.h:217-218
@@ +216,4 @@
+
+ /// \brief Indicates whether
+ /// `getBegin() <= other.getBegin() && other.getEnd() <= getEnd()`.
+ bool contains(SourceRange other) const {
----------------
Richard Smith wrote:
> Rather than essentially duplicating the implementation in the body, how about
> an English description, "Indicates whether another source range is the same
> as, or contained within, this range."
>
> Also, the implementation doesn't appear to be correct; `<` on
> `SourceLocation`s does not in general give source order (though it happens to
> if the locations are within the same `FileID`). Use
> `SourceManager::isBeforeInTranslationUnit` for this check (and it shouldn't
> be a member of `SourceRange` to avoid layering problems).
>
> Maybe this should be a `SourceManager` member function?
I used the short technical definition in the description because I wasn't sure
how to describe the behaviour verbally in a terse and accurate way.
I didn't use `isBeforeInTranslationUnit` for the implementation because I
wanted to test whether one source range is visually contained by the other in
the original source file. However, I just tested the behaviour for macro
expansions and noticed that clang does print both the spelling and the
expansion location for macros, so using `isBeforeInTranslationUnit` indeed
seems preferable here.
Do you maybe have a suggestion for the name of the new method in
`SourceManager`? Maybe `rangeContains`?
================
Comment at: include/clang/Sema/Sema.h:6055-6056
@@ -6054,1 +6054,4 @@
+ ///
+ /// For alias template instantiations this member is used to store the
+ /// source range of the first template argument.
SourceRange InstantiationRange;
----------------
Richard Smith wrote:
> This seems a bit arbitrary. Could you add a `TemplateArgumentListInfo *`
> instead? (Maybe make `TemplateArgs` above a `PointerUnion`?)
Using the `InstantiationRange` for storing the source range seemed like the
minimally invasive way to make the source information for the enable_if
diagnostic available.
I could store a `TemplateArgumentListInfo` pointer instead, but that would also
require a new `InstantiatingTemplate` constructor.
================
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:
> 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.
================
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:
> 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?
http://llvm-reviews.chandlerc.com/D3061
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
