1221368 - MakeTuple doesn't decay the types of its arguments

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Right now MakeTuple does the wrong thing if you pass a variable to it. Example:

int x = 1;
auto t = MakeTuple(x);

The type of t will be Tuple<int&> because of the way perfect forwarding is designed. The C++ standard library fixes this by decaying the types of the arguments of MakeTuple. We should do this too. Otherwise MakeTuple is basically unusable.

Mostly this was pretty easy, but I had a lot of trouble implementing std::is_function. My code works in gcc. clang generates some sort of warning for it, which we could probably work around. But MSVC has serious problems:

template<typename Result, typename... ArgTypes>
// complains here about "error C2143: syntax error : missing ')' before '...'"
struct IsFunction<Result(ArgTypes......)>
  : public TrueType
{};

In addition, this causes errors:

template<typename Result, typename... ArgTypes>
// complains here: "error C2270: 'abstract declarator' : modifiers not allowed on nonmember functions"
struct IsFunction<Result(ArgTypes...) const>
  : public TrueType
{};

I suspect MSVC 2013 just isn't in conformance with the standard. I tried to figure out how is_function is implemented in MSVC's version of type_traits, but I couldn't find a definition there.

Consequently, I just commented this part out. It means we won't correctly decay functions, but I doubt this will bite us in practice. I think it's better to have a slightly broken IsFunction than a totally broken MakeTuple. Definitely open to suggestions though.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

Forgot to attach the patch. See above.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

There's already an IsFunction template:
http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#339
But it's in a strange place and only used there, and yours looks better.

So I'd suggest you update Assertions.h to use your version (so as not to confuse people looking for "IsFunction" and finding two candidates!)

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8682853 [details] [diff] [review]
patch

Review of attachment 8682853 [details] [diff] [review]:
-----------------------------------------------------------------

Assertions.h depends on TypeTraits.h, so IsFunction should definitely move into TypeTraits.h where it belongs.  I have this vague recollection of Assertions.h not having this dependency, wherefore the location, but memory hazy now.

::: mfbt/Tuple.h
@@ +416,5 @@
>   *
>   * auto tuple = MakeTuple(42, 0.5f, 'c');  // has type Tuple<int, float, char>
>   */
>  template<typename... Elements>
> +Tuple<typename Decay<Elements>::Type...> MakeTuple(Elements&&... aElements)

Put an inline in front of the declaration, and move the function name to a new line.  This was/is unreadable.  :-)

::: mfbt/TypeTraits.h
@@ +162,5 @@
>  struct IsArray : detail::IsArrayHelper<typename RemoveCV<T>::Type>
>  {};
>  
> +/**
> + * IsFunction determines whether a type is a function type.

Add some examples, just like all the other doc comments here.  Those should clarify that a function pointer doesn't qualify, only the name of a function symbol, basically.

@@ +180,5 @@
> +
> +/*
> + * This code matches the C++ standard library, but it's commented out because
> + * MSVC++ doesn't seem to support types that combine variadic template arguments
> + * and varargs. It also complains when templates are instantiated with function

Does using RemoveCV<T>::Type not work to eliminate the cv-qualifiers?  Or do those have totally different senses on function types?  (I have no idea.)

@@ +1122,5 @@
>    : detail::RemovePointerHelper<T, typename RemoveCV<T>::Type>
>  {};
>  
> +template<typename T>
> +struct AddPointer

Needs a doc comment like everything else in this file.

@@ +1221,5 @@
> +  typedef typename RemoveReference<T>::Type RemoveType;
> +
> +public:
> +  typedef typename detail::DecaySelector<RemoveType>::Type Type;
> +};

Could also

template<typename T>
struct Decay
  : public detail::DecaySelector<RemoveReference<T>::Type>
{
};

munged however style consistency with everything around demands.

(I didn't look at all whether this implements semantics correctly.)

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8682853 [details] [diff] [review]
patch

Review of attachment 8682853 [details] [diff] [review]:
-----------------------------------------------------------------

I approve of Waldo and Gerald's comments, with the additions noted below.

::: mfbt/TypeTraits.h
@@ +162,5 @@
>  struct IsArray : detail::IsArrayHelper<typename RemoveCV<T>::Type>
>  {};
>  
> +/**
> + * IsFunction determines whether a type is a function type.

In addition to waldo's comments about documentation, tests (mfbt/tests/TestTypeTraits.cpp) would be excellent.

@@ +181,5 @@
> +/*
> + * This code matches the C++ standard library, but it's commented out because
> + * MSVC++ doesn't seem to support types that combine variadic template arguments
> + * and varargs. It also complains when templates are instantiated with function
> + * arguments that use const.

It looks like MSVC uses something like:

template<typename T>
struct is_function : _Is_funptr<typename remove_cv<T>::type *>

with appropriate implementation of _Is_funptr:

template<class _Ret, class... _Args> struct _Is_funptr;

for its implementation of is_function.

I guess this doesn't handle varargs correctly...but I don't know that I'm terribly worried about handling varargs correctly.

@@ +1122,5 @@
>    : detail::RemovePointerHelper<T, typename RemoveCV<T>::Type>
>  {};
>  
> +template<typename T>
> +struct AddPointer

Also needs some tests, along with the documentation.

@@ +1205,5 @@
> +
> +/**
> + * Strips const/volatile off a type and decays it from an lvalue to an
> + * rvalue. So function types are converted to function pointers, arrays to
> + * pointers, and references are removed.

Again, tests.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

I think this addresses all the comments. I'm still not sure exactly what is supposed to happen for something like Decay<void(int) const>, or even IsFunction<void(int) const>. It looks like RemoveConst is not doing what I would expect on that type. Probably not worth too much time unless someone knows what is going on.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8683381 [details] [diff] [review]
patch v2

Review of attachment 8683381 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: mfbt/tests/TestTypeTraits.cpp
@@ +27,5 @@
>  using mozilla::MakeUnsigned;
>  using mozilla::RemoveExtent;
>  using mozilla::RemovePointer;
> +using mozilla::AddPointer;
> +using mozilla::Decay;

Nit: please alphabetize these.

@@ +34,5 @@
> +              "int is not a function");
> +static_assert(IsFunction<void(int)>::value,
> +              "void(int) is a function");
> +static_assert(!IsFunction<void(*)(int)>::value,
> +              "void(*)(int) is not a function");

Nit: maybe "...not a function type"?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a22461c8ce8

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f620224aae8f

B2G JB Emulators both opt and debug:
19:04:43    ERROR -  ../../../../workspace/gecko/ipc/chromium/src/base/task.h:36:4: error: use of deleted function 'RefPtr<T>::operator T*() const && [with T = nsScreenGonk]'

Failed job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a50c676caf7f
Backout job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f620224aae8f

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce5bb1b52a7

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ce5bb1b52a7

Comment 11

[Botond Ballo [:botond]]

(In reply to Bill McCloskey (:billm) from comment #5)
> I'm still not sure exactly what is
> supposed to happen for something like Decay<void(int) const>, or even
> IsFunction<void(int) const>. It looks like RemoveConst is not doing what I
> would expect on that type. Probably not worth too much time unless someone
> knows what is going on.

At the time you wrote this I didn't know about them, but I just came across a paper [1] in the most recent C++ standards committee mailing that talks about "abominable function types", and realized that these fall into that category.

It looks like the answers to your questions are:

  - |Decay<void(int) const>| should be |void(int) const| because the 
    |const| there is not a toplevel-const, but the sort of const that 
    goes at the end of a member function signature. 

  - |IsFunction<void(int) const>::value| should be true.

That said, we can also decide to continue not caring about these "abominations" :)

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0172r0.html