[PATCH] D79800: [Sema] Implement DR2233

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

Thanks for the reviews!
I believe this now handles all cases and with this we're standards-conforming in regard to DR777 and DR2233.



================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1987
+
+    if (Function->getNumParams() >= NumTemplatedParams) {
+      unsigned FirstDefault = 0;
----------------
rjmccall wrote:
> I don't think this simple comparison works; the relationship can be complicated because there might be multiple packs in play.  By the same token, I think it's possible that there can be multiple interleavings of defaultable parameters with packs.  I think you need to scan backwards looking for a parameter without a default argument that was instantiated from a pack expansion and then remove any earlier parameters with defaults.
> 
> That is, assuming this is a reasonable implementation approach at all compared to just teaching other parts of the compiler about this case, which I think Richard needs to weigh in on.
Good point, my current change doesn't work in the following case:


```
template<class... Ts, class... As>
void g(int i = 1, Ts... ts, int j = 3, As... as)
{
}

void testg()
{
	g<int>(2, 3, 4, 5);
}
```

Fixed and added that as a test case.

I think this approach is fine, as keeping info for each parameter for how they were expanded doesn't seem reasonable. Cases such as this ought to be checked and handled early during the actual template instantiation not during later checking of the instantiated function declaration.


================
Comment at: clang/test/CXX/drs/dr7xx.cpp:225
 template <typename... T>
 void f(int i = 0, T ...args) {}
 void ff() { f(); }
----------------
Quuxplusone wrote:
> rjmccall wrote:
> > Quuxplusone wrote:
> > > Is this even supposed to compile? The only valid specializations of `f` require `T...` to be an empty pack, which violates [temp.res/8.3](https://timsong-cpp.github.io/cppwp/temp.res#8.3).
> > > 
> > > The comment mentions [DR777](http://cwg-issue-browser.herokuapp.com/cwg777), but DR777 doesn't explain the circumstances under which its wording change matters. It //seems// only to apply to templates that are already ill-formed by temp.res/8.3.
> > Yeah, Richard made this point in [[http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2233|DR2233]], and the wording was weakened to allow it, in a way that essentially makes the earlier default arguments dead.
> Huh. Ick. Indeed the other vendors seem to be implementing DR777/DR2233, so I guess Clang ought to catch up even if it's a silly direction to go in. :( I do see a small bit of implementation divergence in https://godbolt.org/z/ZMCvAX —
> ```
> template<class... Ts>
> int f(int i=1, Ts... ts) { return (i + ... + ts); }
> 
> template<>
> int f<int>(int i, int j) { return 42; }
> ```
> GCC rejects as ill-formed. MSVC makes the specialization callable with 2 arguments only. EDG (ICC) makes the specialization callable with 0 or 2 arguments (and does [crazy things](https://godbolt.org/z/QTrVeh) when you call it with 0 arguments).
MSVC [[ https://godbolt.org/z/FCtrcs | seems to be the most standard-conforming ]] in this regard. And now with the revised patch Clang is on par. :)

[[ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95287 | GCC bug ]]
[[ https://developercommunity.visualstudio.com/content/problem/1046639/incorrect-intellisense-for-functions-with-a-defaul.html | IntelliSense bug ]]

I also submitted bug reports to ICC, but those seem to need moderator approval before they're public.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79800