[libcxx-commits] [PATCH] D111283: [clang] template / auto deduction deduces common sugar

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:11578
+  default:
+    return X;
+  }
----------------
rsmith wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > For `TemplateArgument::ArgKind::Declaration`, I think it'd make sense to take the common type of the `getParamTypeForDecl`s.
> > Yeah, but this has been dead code since we went back to canonical template parameter / arguments.
> > I have removed it and put some unreachables in place.
> > We will circle back around to add it with this suggestion later.
> This will need to deal with all the cases that can arrive here. If it sees resolved template arguments, then this includes `Type`, `Declaration`, `NullPtr`, `Integral`, `Template`, `TemplateExpansion`, and `Pack`. If it sees unresolved template arguments, then this includes at least `Type`, `Expression`, `Template`, and `TemplateExpansion`. This function currently doesn't cover either set, so I strongly suspect that this `llvm_unreachable` is actually reachable.
> 
> Can we just `return X;` on all the cases we don't currently handle?
Yeah I will just return X for the peace of mind for now, but I think what is happening here is that we only see unresolved template arguments, so we would theoretically be missing the handling of Template and TemplateExpansion cases.

The problem is, I think we are not actually preserving type sugar of those two cases, because of a profiling bug.
I pointed this problem out recently in this revision: https://reviews.llvm.org/D126172

So this is probably pretty hard to test right now, and we would not even get any results from handling it, so I will
leave a FIXME instead.


================
Comment at: clang/lib/AST/ASTContext.cpp:12378-12379
+
+    if (EPIX.EllipsisLoc != EPIY.EllipsisLoc)
+      EPIX.EllipsisLoc = SourceLocation();
+    EPIX.HasTrailingReturn = EPIX.HasTrailingReturn && EPIY.HasTrailingReturn;
----------------
rsmith wrote:
> Can we cope with a variadic function with no ellipsis location? I'd expect that to confuse some parts of Clang. Picking one of the two arbitrarily isn't great but may be the best we can do.
> Can we cope with a variadic function with no ellipsis location? I'd expect that to confuse some parts of Clang. Picking one of the two arbitrarily isn't great but may be the best we can do.

Will leave that out for now with a FIXME.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4715-4716
+             "substituting template parameter for 'auto' failed");
+      if (!Result.isNull())
+        Deduced[0] = DeducedTemplateArgument(Result);
+      if (auto TDK = DeduceTemplateArgumentsFromCallArgument(
----------------
rsmith wrote:
> I do like this approach to repeated deduction, but I'm not sure that it actually works. For example:
> 
> ```
> void f();
> void g();
> void g(int);
> auto h(bool b) {
>   if (b) return f;
>   return g;
> }
> ```
> I think this is required to fail to compile, because we can't deduce the return type from the returned expression `g`. But I think what will happen here is that we'll say that `g` is a non-deduced context and then succeed because we pre-populated the deduced type from the `void(*)()` from `f`.
> 
> Instead, how about not pre-populating `Deduced[0]`, and instead "deducing" the template parameter from `Result` after we check for the `TDK_Incomplete` case below? That should still let us reuse the logic for merging deductions.
Nice find. We don't actually need to repeat the deduction. We only need to return Inconsistent deduction if Result and Deduced[0] are different, and get the common sugar otherwise.

We already had to do so for the `decltype(auto)` case, and we could have totally resugared the initializer lists but forgot as you pointed out, and we had to make this same check for that case anyway.

So I combined all these cases, further simplifying things, and added the new test case for initializer_list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283