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] Multiple T data members before this data member #334

Closed
JohelEGP opened this issue Apr 8, 2023 · 8 comments
Closed

[BUG] Multiple T data members before this data member #334

JohelEGP opened this issue Apr 8, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 8, 2023

Direct inheritance from the same type cpp2::store_as_base<T> multiple times is ill-formed. I suggest its API follows https://www.boost.org/doc/libs/release/libs/utility/doc/html/utility/utilities/base_from_member.html.

Minimal reproducer:

x: type = { }
w: type = {
  a: std::string;
  b: std::string;
  this: x;
}

Commands:

cppfront x.cpp2
clang++17 -std=c++2b -I $CPPFRONT_INCLUDE_DIR x.cpp

Expected result: A well-formed program.

Actual result and error:

Generated C++1
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"

#line 1 "x.cpp2"
class x;
class w;

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

#line 1 "x.cpp2"
class x {};
class w: private cpp2::store_as_base<std::string>, private cpp2::store_as_base<std::string>, public x {

#line 6 "x.cpp2"
};

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


x.cpp2:2:52: error: base class 'cpp2::store_as_base<std::string>' (aka 'store_as_base<basic_string<char>>') specified more than once as a direct base class
class w: private cpp2::store_as_base<std::string>, private cpp2::store_as_base<std::string>, public x {
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
@JohelEGP JohelEGP added the bug Something isn't working label Apr 8, 2023
@JohelEGP
Copy link
Contributor Author

JohelEGP commented Apr 8, 2023

For

w: type = {
  a: T;
  b: U;
  this: x;
}

I think, in general, cppfront can't know whether T and U are the same type. So something like the UniqueID part of boost::base_from_member would have to be used always.

@hsutter
Copy link
Owner

hsutter commented Apr 8, 2023

Right, the next thing I wanted to add was a disambiguator for exactly that case... hold on...

@hsutter
Copy link
Owner

hsutter commented Apr 8, 2023

(I meant to do that before pushing this commit... I knew there was one thing I'd forgotten...)

@hsutter
Copy link
Owner

hsutter commented Apr 8, 2023

Actually the idea I had in mind is simpler... move the String template-parameter-friendly helper from common.h to cpp2util.h so cpputil can use it, and then add it as a parameter to store_as_base and just store the actual member name (e.g., store_as_base<"name", std::string> and store_as_base<"address", std::string> should be fine as well as more readable than a generated unique ID).

@hsutter hsutter closed this as completed in fe13207 Apr 8, 2023
@filipsajdak
Copy link
Contributor

@hsutter So, from what I understand, if I declare a member before a base type, the member will be put into store_as_base, and it will be privately inherited.

As this is perfectly fine (especially when the name will be added as a template parameter) I am curious about what is behind that idea. Is there any rule that covers that or is it just an enabler for something else?

@hsutter
Copy link
Owner

hsutter commented Apr 8, 2023

Done -- BTW @JohelEGP I didn't know about Boost's base_from_member but that doc page has a nice motivating example of when you want to do this.

So I think Cpp2 now allows both examples from that page's documentation, but expressed directly. This is pretty nice, here's how I understand it (please correct, I didn't compile the following code so it may have typos/thinkos)...

Example 1: Single object

In the Boost example, the first motivating example looks like this, where you need to initialize the output streambuf-derived member before passing it to initialize the base class ostream:

Today, using Boost

class fdostream
  : private boost::base_from_member<fdoutbuf>
  , public std::ostream
{
    // Helper typedef's
    typedef boost::base_from_member<fdoutbuf>  pbase_type;
    typedef std::ostream                        base_type;

public:
    explicit fdostream( int fd )
      : pbase_type( fd ), base_type( &member ){}
    //...
};

Cpp2 equivalent

As of today's cppfront commits that add inheritance, and allow ordering member and base subobjects interleaved and accessing them all by name, in Cpp2 you just write the (data/base) members in the desired order and use their names directly:

fdostream: type = {
    buf: fdoutbuf;
    this: std::ostream;

    operator=: (out this, int fd ) = {
        buf = fd;
        std::ostream = buf&;
    }
    //...
};

Example 2: Multiple objects

Here's the second example from that Boost page, which shows a little state machine:

Today, using Boost

class system
  : private boost::base_from_member<an_int>
  , private boost::base_from_member<switcher>
  , private boost::base_from_member<switcher, 1>
  , private boost::base_from_member<switcher, 2>
  , protected flow_regulator
  , public fan<6>
{
    // Helper typedef's
    typedef boost::base_from_member<an_int>       pbase0_type;
    typedef boost::base_from_member<switcher>     pbase1_type;
    typedef boost::base_from_member<switcher, 1>  pbase2_type;
    typedef boost::base_from_member<switcher, 2>  pbase3_type;
    typedef flow_regulator  base1_type;
    typedef fan<6>          base2_type;

public:
    system( double x );
    //...
};

system::system( double x )
  : pbase0_type( 0.2 )
  , pbase1_type()
  , pbase2_type( -16, &this->pbase0_type::member.y )
  , pbase3_type( x, static_cast<int *>(NULL) )
  , base1_type( pbase3_type::member, pbase1_type::member )
  , base2_type( pbase2_type::member )
{
    //...
}

Cpp2 equivalent

If I understand the Boost example correctly, the equivalent in Cpp2 as of today is now this (modulo typos):

system: type = {
    state0: an_int;
    state1: switcher,
    state2: switcher,
    state3: switcher,
    this: flow_regulator // note: public, not protected
    this: fan<6>;

    operator=: (out this, x: double ) = {
        state0 = 0.2;
        state1 = ();
        state2 = ( -16, state0.y& );
        state3 = ( x, nullptr );
        flow_regulator = ( state3, state1 );
        fan<6> = state2;
        //...
    }
    //...
};

Nice, thanks for the Boost link.

@hsutter
Copy link
Owner

hsutter commented Apr 8, 2023

@hsutter So, from what I understand, if I declare a member before a base type, the member will be put into store_as_base, and it will be privately inherited.

Yes. The latter half of the sentence is an implementation detail -- from the point of view of the user, they simply wrote the data member subobject before the base subobject, which not only guarantees they are laid out in that order but also guarantees that the data member is initialized (constructed) before the base class.

As this is perfectly fine (especially when the name will be added as a template parameter) I am curious about what is behind that idea. Is there any rule that covers that or is it just an enabler for something else?

See the above examples (thanks again for the Boost page link @JohelEGP!)... it doesn't come up a lot, but sometimes there is a need to initialize a base class with a data member, and they'd better be constructed in the right order. I knew workarounds for this existed, but I didn't know Boost had one, and that what I just checked in pretty much mirrored the Boost implementation (wow... because I considered several different implementations and decided the private wrapped base one was best, and lo and behold! Daryle Walker, the author of this Boost library, made the same decision -- thanks Daryle!).

Also, pasting the acknowledgements from the Boost page:

Those give some additional references/uses. I confess that when I decided to support interleaved bases and (other) members, I didn't know about these specific examples. I just knew about the problem and that there were use cases (I've mentioned this a few times, at least as early as a 1998 C++ Report article of mine), and I knew that unifying inheritance syntax (so that base classes weren't declared in a separate base class list and weren't initialized in a separate member initializer list) would naturally allow interleaving them with other members, that that was a semantically powerful generalization (not just a syntactic convenience), and that supporting the interleaving was likely a small use case but one worth supporting from the outset as soon as I added inheritance (in particular, it directly hits initialization safety, so even though it's a rarer need, it's a safety-important one).

Sorry for the long sentences and parentheticals, I have time to write but not time to edit. 😉 Obligatory Pascal (the person) quote reference here

@filipsajdak
Copy link
Contributor

Thanks for the explanation!

zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
…se`, closes hsutter#334

Disambiguates two such members having the same type

Move the existing `String` wrapper into `cpp2util.h` where it can be used outside the compiler too, in this case by `store_as_base`
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

3 participants