Patch LGTM, thanks!
On Mon, Jun 3, 2013 at 3:35 PM, Larisse Voufo <[email protected]> wrote: > > I am attaching a revision of the patch based on the last comments. > > > On Thu, May 30, 2013 at 5:54 PM, Richard Smith <[email protected]> > wrote: >> >> On Thu, May 30, 2013 at 4:35 PM, Larisse Voufo <[email protected]> wrote: >>> >>> Please see attached. >> >> >> This looks really good. >> >> Something funny has happened around < >s for templates. I've run the patch >> through clang-format for you (attached). > > > Thanks. It looks like my editor cannot tell template arguments' brackets > from binary operators, and defaults into the later option. I'll be more > careful about that. > >> >> >> + // Fixme: this assumes that both types will be equally >> + // restrict-qualified. >> >> s/Fixme/FIXME/ >> >> >> >> + if (ToType.isMoreQualifiedThan(CurToType)) >> + ToType = CurToType; >> >> The standard doesn't seem very clear on this, but I think you should merge >> qualifiers here rather than taking the more-qualified type. If we have a >> conversion to 'const T' and a conversion to 'volatile T', our current choice >> between them seems to depend on the order in which we see them. >> > Ah. Good point. Following this train or reasoning, we should be looking for > the least common qualifier between the conversion candidates. > However, as you pointed out earlier, the standard does suggest to just drop > the qualifiers. I have made the change (cf. attached). > >> >> >> + if (ConvTemplate) >> + AddTemplateConversionCandidate(ConvTemplate, FoundDecl, >> ActingContext, >> + From, ToType, CandidateSet); >> >> It looks like ViableConversions won't contain any conversion operator >> templates at this point. However, we should probably be performing overload >> resolution on the complete set of conversions, not just on the ones we >> matched earlier, > > > Right. I have changed the meaning of ViableConversions for C++1y so that it > contains *potentially* viable conversions, i.e. including conversion > templates as well (again, cf. attached). > >> >> for oddball cases like: >> >> struct A { >> operator int() &&; >> template<typename T> operator T(); >> }; >> void f(A a) { >> switch (a) {} // should presumably call templated conversion operator >> to convert to int. >> } >> struct B { >> operator bool() &&; >> operator void*(); >> }; >> void g(B b) { >> switch (b) {} // should presumably call 'b.operator void*()' then >> convert result to bool >> } >> >> The patch could do with more testcases, covering more exotic situations >> such as the above, and the new error cases. > > Sure. Working on that. I've added a unit tests. More test cases are welcome. > :) > The above example is now covered. > >> >> >> Thanks! >> Richard > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
