Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Can't author trivial functions #547

Open
JohelEGP opened this issue Jul 17, 2023 · 4 comments
Open

[BUG] Can't author trivial functions #547

JohelEGP opened this issue Jul 17, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

Title: Can't author trivial functions.

Description:

Currently, Cppfront generates special member functions even when a =defaulted one would do.

When a Cpp2 operator= for a move/copy constructor/assignment or destructor
would perform the same operations as a =defaulted one,
it should be lowered as =defaulted.

Minimal reproducer (https://cpp2.godbolt.org/z/4nozcbdcf):

template<class T> struct union_ {
  union {
    bool nullopt = true;
    T value;
  };
  union_() { }
  union_(const union_&) requires std::is_trivially_copy_constructible_v<T> = default;
  ~union_() requires std::is_trivially_destructible_v<T> = default;
  union_& operator=(const union_&) requires std::is_trivially_copy_assignable_v<T> = default;
  union_(const union_&) { }
  void operator=(const union_&) { }
  ~union_() { }
};
optional: <T> type = {
  has_value: bool = false;
  this: union_<T> = ();
  operator=: (out this) = { }
  // Comment out, pending fix to #323.
  // operator=: (out this, that) requires std::is_trivially_copy_constructible_v<T> = { }
  // operator=: (out this, move that) requires std::is_trivially_move_constructible_v<T> = { }
  // operator=: (inout this, that) requires std::is_trivially_copy_assignable_v<T> = { }
  // operator=: (inout this, move that) requires std::is_trivially_move_assignable_v<T> = { }
  // operator=: (move this) requires std::is_trivially_destructible_v<T> = { }
  private move_construct: (inout this, forward other) = {
    if other.has_value {
      has_value = true;
      this.value = (move other).value;
    }
  }
  operator=: (out this, that) = move_construct(that);
  operator=: (out this, move that) = move_construct(move that);
  private move_assign: (inout this, forward other) = {
    if has_value && other.has_value {
      this.value = (move other).value;
    } else if has_value {
      std::destroy_at(this.value&);
      this.nullopt = true;
    } else if other.has_value {
      has_value = true;
      std::construct_at(this.value&, (move other).value);
    }
  }
  operator=: (inout this, that) = move_assign(that);
  operator=: (inout this, move that) = move_assign(move that);
  operator=: (move this) = { if has_value { std::destroy_at(this.value&); } }
}
test: <T> () = {
  x: optional<T> = ();
  y := x;
  _ = :optional<T> = (move x);
  x = y;
  x = y;
}
main: () = {
  test<i32>();
  test<std::string>();
  static_assert(std::is_trivially_destructible_v<union_<i32>>);
  [[assert Testing: std::is_trivially_destructible_v<optional<i32>>]]
}
Commands:
cppfront main.cpp2
clang++17 -std=c++23 -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -I . main.cpp

Expected result:

After uncommenting // operator=: (move this) requires std::is_trivially_destructible_v<T> = { },
for the assertion to pass.

Actual result and error:

Cpp2 lowered to Cpp1:
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"


template<typename T> class optional;
  

//=== Cpp2 type definitions and function declarations ===========================

template<class T> struct union_ {
  union {
    bool nullopt = true;
    T value;
  };
  union_() { }
  union_(const union_&) requires std::is_trivially_copy_constructible_v<T> = default;
  ~union_() requires std::is_trivially_destructible_v<T> = default;
  union_& operator=(const union_&) requires std::is_trivially_copy_assignable_v<T> = default;
  union_(const union_&) { }
  void operator=(const union_&) { }
  ~union_() { }
};

struct optional_has_value_as_base { bool has_value; };
template<typename T> class optional: public optional_has_value_as_base, public union_<T> {

  public: explicit optional();
  // Comment out, pending fix to #323.
  // operator=: (out this, that) requires std::is_trivially_copy_constructible_v<T> = { }
  // operator=: (out this, move that) requires std::is_trivially_move_constructible_v<T> = { }
  // operator=: (inout this, that) requires std::is_trivially_copy_assignable_v<T> = { }
  // operator=: (inout this, move that) requires std::is_trivially_move_assignable_v<T> = { }
  // operator=: (move this) requires std::is_trivially_destructible_v<T> = { }
  private: auto move_construct(auto&& other) -> void;
    

  public: optional(optional const& that);
  public: optional(optional&& that) noexcept;
  private: auto move_assign(auto&& other) -> void;
    

  public: auto operator=(optional const& that) -> optional& ;
  public: auto operator=(optional&& that) noexcept -> optional& ;
  public: ~optional() noexcept;
};
template<typename T> auto test() -> void;
  

auto main() -> int;
  

//=== Cpp2 function definitions =================================================


  template <typename T> optional<T>::optional()
                            : optional_has_value_as_base{ false }
                            , union_<T>{  }
  {}

  template <typename T> auto optional<T>::move_construct(auto&& other) -> void{
    if (other.has_value) {
      has_value = true;
      (*this).value = (std::move(CPP2_FORWARD(other))).value;
    }
  }
  template <typename T> optional<T>::optional(optional const& that)
                                : optional_has_value_as_base{ that.has_value }
                                , union_<T>{ that }
   { move_construct(that);  }
  template <typename T> optional<T>::optional(optional&& that) noexcept
                                     : optional_has_value_as_base{ std::move(that).has_value }
                                     , union_<T>{ std::move(that) }
   { move_construct(std::move(std::move(that)));  }
  template <typename T> auto optional<T>::move_assign(auto&& other) -> void{
    if (has_value && other.has_value) {
      (*this).value = (std::move(other)).value;
    }else {if (has_value) {
      std::destroy_at(&(*this).value);
      (*this).nullopt = true;
    }else {if (other.has_value) {
      has_value = true;
      std::construct_at(&(*this).value, (std::move(CPP2_FORWARD(other))).value);
    }}}
  }
  template <typename T> auto optional<T>::operator=(optional const& that) -> optional&  { 
                                  has_value = that.has_value;
                                  union_<T>::operator= ( that );
  move_assign(that);
                                  return *this;
   }
  template <typename T> auto optional<T>::operator=(optional&& that) noexcept -> optional&  { 
                                       has_value = std::move(that).has_value;
                                       union_<T>::operator= ( std::move(that) );
  move_assign(std::move(std::move(that)));
                                       return *this;
   }
  template <typename T> optional<T>::~optional() noexcept{if (has_value) {std::destroy_at(&(*this).value); }}

template<typename T> auto test() -> void{
  optional<T> x {}; 
  auto y {x}; 
  (void) optional<T>{std::move(x)};
  x = y;
  x = std::move(y);
}
auto main() -> int{
  test<cpp2::i32>();
  test<std::string>();
  static_assert(std::is_trivially_destructible_v<union_<cpp2::i32>>);
  cpp2::Testing.expects(std::is_trivially_destructible_v<optional<cpp2::i32>>, "");
}
Output:
Testing violation
libc++abi: terminating
@JohelEGP JohelEGP added the bug Something isn't working label Jul 17, 2023
@JohelEGP
Copy link
Contributor Author

As an opt-out of emitted Cpp1 =default in case triviality is undesirable,
how about supporting an explicit empty statement with the placeholder _;?
The alternative is for everyone to use their own "do nothing" statement.

Then this Cpp2:

operator=: (out this) = { }          // 1
operator=: (out this) = _;           // 2
while do_stuff().and_continue { _; } // 3

Can lower to this Cpp1:

t() = default;                      // 1
t() { }                             // 2
while (do_stuff().and_continue) { } // 3

@realgdman
Copy link

I like idea, but why not other way around? = { } emits { }, and = _ emits = default, because it is already close to meaning "insert what fits here and I don't care what exactly it is"

@JohelEGP
Copy link
Contributor Author

while do_stuff().and_continue { _; } // 3 is silly.
{ } is already allowed.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Nov 3, 2023

Or add a @trivial metafunction that lowers to = default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants