[BUG] Improve `cpp2::in` (restore old behavior, exclude problem cases only)

[JohelEGP]

Describe the bug

#248 (comment) finally pushed to comment on this. This formulation:

• Passes fundamental types of size greater than 2 object pointers, like some 256-bit extended integer type.
• Introduces a regression. Types that are not fundamental due to ABI issues, like GCC's 128-bit integer types (-pedantic), are passed by const&.

I suggest reverting the deletion of prefer_pass_by_value and enhancing it to exclude the problematic incomplete types that can affect cpp2::in.

-- a89af8f#r107115865

Also fix < to <= to consider types exactly the size of 2 object pointers. See https://compiler-explorer.com/z/8EKPMrhee.

[JohelEGP]

Here's a table to visualize the in types taken by value vs. by reference
before a89af8f, since a89af8f, and proposed here (#317),
assuming

• the "fix < to <=" above, and
• an implementation with
  • sizeof(void*) == 8, 64 bits (CHAR_BIT == 8), and
  • some hypothetical integer types.

i128 i256 // Extended integer types.
__i128 __i256 // Non-fundamental integer types.
enum enum128 : std::int128_t { }; // C++1
enum enum256 : std::int256_t { }; // C++1
my_t: type = { }
my_t[] my_t[3]
void()

The first column uses ❌ for the bad cases before a89af8f.
The other columns use ❌ and ✔ for the improvements relative to before a89af8f.

Before a89af8f	Since a89af8f	#317
i128 (copy)	i128 (copy)	i128 (copy)
i256 &	i256 (copy) ❌	i256 &
__i128 (copy)	__i128 & ❌	__i128 (copy)
__i256 &	__i256 &	__i256 &
enum128 (copy)	enum128 (copy)	enum128 (copy)
enum256 &	enum256 (copy) ❌	enum256 &
my_t (copy) ❌	my_t & ✔	my_t & ✔
my_t[] error ❌	my_t[] & ✔	my_t[] & ✔
my_t[3] &	my_t[3] &	my_t[3] &
void() error ❌	void() & ✔	void() & ✔
void errors ❌	void error ❌	void constraint ✔

a89af8f's commit message mentions:

Changed in<T> to pass fundamental types by value. Only those are known to be always complete. Partly addresses #270.

In no discussion was it even hinted at to also disregard the size part from the convention:

• (Simple) ((Foundation)) Warn when a parameter being passed by value has a size greater than 2 * sizeof(void*).
  Suggest using a reference to const instead.
  -- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

As shown in the table, since a89af8f,

• size is not taken into account, so types of size greater than 2 * sizeof(void*) are passed by value, and
• types that can never be incomplete are always passed by reference to const, even if their size is not greater than 2 * sizeof(void*).

Bonus points: #317 (comment).

[JohelEGP]

Based on the discussion at a89af8f#r107115865 and below, I propose this new formulation for cpp2::in:

template<typename T>
constexpr bool prefer_pass_by_value =
    sizeof(T) <= 2*sizeof(void*)
    && std::is_trivially_copy_constructible_v<T>;

template<typename T>
    requires std::is_class_v<T> || std::is_union_v<T> || std::is_array_v<T> || std::is_function_v<T>
constexpr bool prefer_pass_by_value<T> = false;

template<typename T>
    requires (!std::is_void_v<T>)
using in =
    std::conditional_t<
        prefer_pass_by_value<T>,
        T const,
        T const&
    >;

[gregmarr]

Unfortunately, it doesn't look like is_trivially_copy_constructible_v can be used here:

https://en.cppreference.com/w/cpp/types/is_copy_constructible

T shall be a complete type, (possibly cv-qualified) void, or an array of unknown bound. Otherwise, the behavior is undefined.

Maybe the requires qualified version is enough to block everything that isn't those types. If so, then maybe this would work too.

template<typename T>
constexpr bool prefer_pass_by_value =
    sizeof(T) <= 2*sizeof(void*)
    && !std::is_class_v<T> 
    && !std::is_union_v<T>
    && std::is_trivially_copy_constructible_v<T>;

[JohelEGP]

You must exclude the cases where T can be incomplete without sizeof and std::is_trivially_copy_constructible_v requiring to be valid, as those require a complete type. See https://compiler-explorer.com/z/8ejEG9qqK.

[gregmarr]

So that's the reason for the two versions, there's no short-circuiting there, and is_class_v and is_union_v covers everything that can be an incomplete type?

Nope, also need || std::is_unbounded_array_v<T> to handle typedef int A[];.

Also doesn't handle typedef S AS[4]; from https://godbolt.org/z/4hW583f8o

[JohelEGP]

See a89af8f#r107716660. Can you prove they affect cpp2::in?

[filipsajdak]

@JohelEGP I have done a small test with your proposed solution: https://godbolt.org/z/qnYaYcxPM

auto fun(in<int>) {} // works
auto fun(in<S>)   {} // works
auto fun(in<C>)   {} // works
auto fun(in<E>)   {} // works
auto fun(in<U>)   {} // works

auto fun(in<A>)   {} // fails, but works with std::is_scalar_v
auto fun(in<AS>)  {} // fails, but works with std::is_scalar_v
auto fun(in<V>)   {} // fails, need to add requires (!std::is_void_v<T>)

[filipsajdak]

We can only try to declare them: https://godbolt.org/z/sn11YdY6f

auto fun(in<int>); // works
auto fun(in<S>);   // works
auto fun(in<C>);   // works
auto fun(in<E>);   // works
auto fun(in<U>);   // works

auto fun(in<A>);   // fails, but works with std::is_scalar_v
auto fun(in<AS>);  // fails, but works with std::is_scalar_v
auto fun(in<V>);   // fails, need to add requires (!std::is_void_v<T>)

The types might comes from cpp1 we might don't know what they are alias for, or it just might be inside some generic code (that is why I have tested it against void).

[filipsajdak]

TLDR: currently (probably) the most reliable version is:

template<typename T>
    requires (!std::is_void_v<T>)
using in =
    std::conditional_t<
        std::is_scalar_v<T>,
        T const,
        T const&
    >;

[JohelEGP]

The types might comes from cpp1 we might don't know what they are alias for, or it just might be inside some generic code (that is why I have tested it against void).

You may be right about void. It's actually legal, but I'll leave it to Herb to decide how to handle that.

See https://compiler-explorer.com/z/Tq65osnof.

auto fun(in<A>)   {} // works
auto fun0(A) { }
void g(A a) { fun0(a); } // works

auto fun(in<AS>)  {} // works? should it?
auto fun1(AS) { }
void g(AS a) { fun1(a); } // works

auto fun(in<V>)   {} // works, should it?
auto fun2(void) { } // works
void g() { fun2(); }

So those work, but not when going through cpp2::in as previously defined. I think we need to unconditionally include arrays of unknown bound, as mentioned at the end of a89af8f#r107715926. I'm not so sure about the semantics of bounded arrays as arguments, but they seem to work, so they could be conditionally included. I also included void because it's legal.

[JohelEGP]

I thought a bit more. in void doesn't make sense, so let's take your suggestion of preventing that with a requires-clause. I fixed prefer_pass_by_value to true for arrays. I think they decay to pointers as parameters. See the mangled signatures for fun0 and fun1:

• GCC fun0(int*) fun1(S*)
• Clang fun0(int*) fun1(S*)
• MSVC fun0(int * const) fun1(S * const)

I'll amend #317 (comment). See https://compiler-explorer.com/z/K1TsT7aeG.

[JohelEGP]

As a parameter,

• an array decays to a pointer, and
• a reference to an array remains a reference to an array.

in x: T declares the intent to take a T as a parameter. If T is an array, x should be (a reference to) an array. A pointer is not an array, and cpp2::in<T> should reflect this.

I'll amend #317 (comment). See https://compiler-explorer.com/z/TEe98e171.

[JohelEGP]

Made #317 (comment) table shorter and nicer, and added:

Before a89af8f	Since a89af8f	#317
my_t[] ❌ (error)	my_t[] & ✔	my_t[] & ✔
my_t[3] &	my_t[3] &	my_t[3] &

Test the columns at https://compiler-explorer.com/z/d3x3TfGP7.