[PATCH] D133683: [c++] implements tentative DR1432 for partial ordering of function template

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a subscriber: hubert.reinterpretcast.
ychen added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5455-5456
     if (!ClangABICompat15) {
-      // Consider this a fix for CWG1432. Similar to the fix for CWG1395.
       auto *TST1 = T1->castAs<TemplateSpecializationType>();
       auto *TST2 = T2->castAs<TemplateSpecializationType>();
+      const TemplateArgument &TA1 = TST1->template_arguments().back();
----------------
mizvekov wrote:
> ychen wrote:
> > mizvekov wrote:
> > > ychen wrote:
> > > > ychen wrote:
> > > > > mizvekov wrote:
> > > > > > This is a bug, T1 and T2 are not in general canonical types for this function, and this castAs can pick up an alias template, and these should never participate in deduction.
> > > > > > 
> > > > > > (Also, can you please add a test for that?)
> > > > > > 
> > > > > > You should not need to keep sugar here, everything being tested below is present on the canonical type, so this is simple to solve, as suggested.
> > > > > I see. I'll add a test.
> > > > Looking more closely, `T1`/`T2` is mostly returned by `ASTContext::getTemplateSpecializationType` and for injected template specialization, always unqualified   https://github.com/llvm/llvm-project/blob/52dce8900c46d5842a021619537ede598983dfde/clang/include/clang/AST/Type.h#L5431-L5432 , it does not seem possible to have aliase template involved? I could've missed something though.
> > > Yeah you are right, I missed that all the uses of this function, those types either come from an Injected TST, or a dependent TST created on the spot.
> > > 
> > > But yeah, the change to a simple `cast` instead of `castAs` makes this more clear.
> > > 
> > > Otherwise, if there could have been any sugar node on top of the TST, that sugar could have easily been an alias TST,
> > > and I think it's very unlikely one wouldn't care, as they are very different things.
> > > 
> > > So I tend to view these as potential bugs, a simple `getAs` and `castAs` on TSTs, which does not check or keeps digging through alias templates is highly suspicious :)
> > > 
> > > Thanks for taking a look at this though!
> > Thanks for the explanation. Yeah, `castAs` looks misleading indeed. I wasn't aware of the subtle difference between `cast` and `castAs`. BTW, are you comfortable accepting this patch?
> I hope I answered that in my other comment, but since this is a speculative wording fix, I think it's more reasonable to wait more than a week for other reviewers, specially since folks can be on time off and such.
> 
> But lacking interest from other folks in reviewing this, yeah sure.
Thanks, that makes sense. I wasn't aware of your other comments when writing this.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5179-5231
   if (!Better1 && !Better2) // Neither is better than the other
     return JudgeByConstraints();
 
   // C++ [temp.deduct.partial]p11:
   //   ... and if G has a trailing function parameter pack for which F does not
   //   have a corresponding parameter, and if F does not have a trailing
   //   function parameter pack, then F is more specialized than G.
----------------
mizvekov wrote:
> ychen wrote:
> > mizvekov wrote:
> > > One thing I have been thinking about all of these "more constrained" checks, is whether we should actually be performing them over the results of the deductions for the least constrained checks.
> > > 
> > > Ie especially with regards to this new check and the one for `[temp.deduct.partial]p11:`, it seems that we should be instead considering as more constrained the deduction which deduced a lesser amount of template parameters, including ones in packs. Or something along this line.
> > > 
> > > This way, we do one sweeping general rule to handle all these cases, instead of just appending these ad-hoc tie-breaking rules which look more like workarounds.
> > > 
> > > What do you think about that?
> > > 
> > > But this is a big change and I don't want to block you on that. Because otherwise, this new change is keeping in line with other workarounds that have already been added here, so this is within existing practice.
> > > 
> > > Since this patch was submitted quite recently and it's a speculative fix, I would let this patch marinate a little more and wait for any other reviewers.
> > > Ie especially with regards to this new check and the one for [temp.deduct.partial]p11:, it seems that we should be instead considering as more constrained the deduction which deduced a lesser amount of template parameters, including ones in packs. Or something along this line.
> > >
> > >This way, we do one sweeping general rule to handle all these cases, instead of just appending these ad-hoc tie-breaking rules which look more like workarounds.
> > >
> > > What do you think about that?
> > 
> > I think you meant the cases that for a  `P`/`A` deduction pair, either `P`/`A` is pack and the other is not empty, right? If so, it already works that way. https://eel.is/c++draft/temp.deduct#type-9
> > 
> > If neither `P`/`A` is pack and either is empty, then they wouldn't get into the stage of partial ordering at all because the function arguments list can only have one length,  `P`/`A` would not be in the same candidate sets.
> > 
> > Here handles the case that either  `P`/`A` is pack and the other is empty. Then the deduction rule could not kick in because one of the `P`/`A`pair is none. So we have to tie-break after the deduction.
> I understand we need to tie break after the at-least-as-specialized check, but I was wondering if we could come up with one sweeping rule / mechanism, which incorporates not only these two cases but also the concept more-constrained check, instead of appending multiple completely separate tie breakers.
> 
> Otherwise, I think this gets pretty hard to incorporate into your mental model of what overload resolution should pick up and such.
> 
> For example, even the order which we tie break seems arbitrary, and this makes it harder for folks to learn / memorize them and for them to actually be useful, instead of just programming traps. But this is C++ so I may be barking at the wrong tree.
The rules are complicated and we should definitely simplify these if possible. I'll give it more thought. @hubert.reinterpretcast worked a lot in this area. If he is interested, may shed some light here.


================
Comment at: clang/test/CXX/drs/dr6xx.cpp:1104
       f<int>(42); // expected-error {{ambiguous}}
       g(42); // expected-error {{ambiguous}}
     }
----------------
mizvekov wrote:
> I would expect the call to `g` not to be ambiguous here, in the same vein of keeping with the spirit of the rules.
> 
> If you agree, can we at least add a FIXME to make a note of that?
Yeah, I agree it is intuitive to work that way. I think it is not in the standard right now because it does not have a strong use case and it requires a big change because right now deduction for partial ordering and the following tie-breakers do not consider the function template parameter list at all, if we do this, then all partial orderings and template specializations are affected. 

The use case of CWG1432 is partial specialization which is very important and easier to catch attention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133683