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] public non-this data member has no effect #336

Closed
JohelEGP opened this issue Apr 9, 2023 · 5 comments
Closed

[BUG] public non-this data member has no effect #336

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

Comments

@JohelEGP
Copy link
Contributor

JohelEGP commented Apr 9, 2023

public this: is disallowed. But before a this data member, a public non-this data member is not diagnosed, and has no effect.

Minimal reproducer:

x: type = { }
y: type = { }
z: type = {
  public a: x = x();
  this: y = ();
  operator=: (out this) = { }
}
w: x = z().a;

Commands:

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

Expected result: A well-formed program, or a diagnostic.

Actual result and error:

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


#include "cpp2util.h"

#line 1 "x.cpp2"
class x;
class y;
class z;

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

#line 1 "x.cpp2"
class x {};
class y {};
class z: private cpp2::store_as_base<"a",x>, public y {

#line 6 "x.cpp2"
  public: z();
};
extern x w;
//=== Cpp2 function definitions =================================================


#line 6 "x.cpp2"
  z::z()
                            : cpp2::store_as_base<"a",x>{ x() }
#line 6 "x.cpp2"
  {}

x w {z().a}; 
x.cpp2:8:10: error: no member named 'a' in 'z'
x w {z().a}; 
     ~~~ ^
1 error generated.
@JohelEGP JohelEGP added the bug Something isn't working label Apr 9, 2023
@JohelEGP
Copy link
Contributor Author

JohelEGP commented Apr 9, 2023

If the position of a non-this data member, with respect to a this data member, shouldn't change behavior, perhaps the generation of a base class is the answer:

struct cpp2_a_for_z {
  x a;
};
class z : public cpp2_a_for_z // ...

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Apr 9, 2023

I noticed that public does affect the following data member.

  public a: x = x();
  this: y = ();
class z: private cpp2::store_as_base<"a",x>, public y {

@hsutter
Copy link
Owner

hsutter commented Apr 9, 2023

[Updated]

Thanks!

perhaps the generation of a base class is the answer:

Hmm, you may have something there. I thought of several ways to implement this, but didn't think of that one -- thank you.

[ETA] On second thought, I wonder if I would then have to echo the constructors the same way again, and that would lead to a lot more generated code. For now, perhaps simpler is to disallow non-private data members before base types... that should be a weird combination.

I noticed that public does affect the following data member.

No, that one is just because my current experiment is to require that all base types are public, so this: y is implicitly public (and can't be declared with any other accessibility).

@hsutter hsutter closed this as completed in f83ca9b Apr 9, 2023
@JohelEGP
Copy link
Contributor Author

On second thought, I wonder if I would then have to echo the constructors the same way again, and that would lead to a lot more generated code.

How about using an aggregate base class that holds a T? cpp2::store_as_base, as currently specified, introduces an extra indirection in the initialization:

a: type = {
  b: int = 0;
}
// Directly initializes `b` from `0`.
c: a = ();

// If we could make an aggregate:
// d: type = {
//   public e: int;
// }
// // Directly initializes `e` from `0`.
// f: d = (0);

g: type = {
  h: int;
  operator=: (out this, x: int) = h = x;
}
// Initializes `x` from `0`, then `h` from `x`.
i: g = 0;

j: type = { }
k: type = {
  l: int = 0;
  this: j = ();
  m: int = 0;
  operator=: (out this) = { }
  operator=: (out this, x: int, y: int) = {
    l = x;
    m = y;
  }
}
// Initializes `QoI` from `0`, then `l` from `std::move(QoI)`.
// Directly initializes `m` from `0`.
n: k = ();
// Initializes `x` from `0`, then `QoI` from `x`, then `l` from `std::move(QoI)`.
// Initializes `y` from `1`, then `m` from `y`.
o: k = (0, 1);

hsutter added a commit that referenced this issue Apr 14, 2023
Replace `store_as_base` with generated aggregate bases - better fix for #336, thanks @JohelEGP for the suggestion! This way we also don't need to obfuscate the name anywhere beyond the constructor(s), as the right member object name just enters the class's scope

If the user didn't write a constructor, provide a default constructor

If the user didn't write a 'that' constructor, suppress Cpp1's compiler-generated copying and assignment

Clean up emission of the just-mentioned generated/=deleted constructors, more naturally line up inside the class body following the indentation for other members that the original source code uses

Rename file `load.h` to `io.h` (`file.h` was another candidate), just because it has been bothering me for a year now that except for that one file all the headers were in natural alphabetical order by compilation phase... as of this commit we now have them all in alpha order and phase order: common.h -> io.h -> lex.h -> parse.h -> [*] -> sema.h -> cppfront.h

    [*] coming next here: reflect.h, which will also be in both alpha order and compilation order

Guard `out.construct` with `if constexpr` in case the type is not copy assignable and that path is never requested

Rename `cpp2::error` to `cpp2::error_entry` to quiet a new(? why?) GCC message about shadowing the former name with `parser::error`... I get why the warning is there, but this is a slightly annoying warning to have to satisfy just to compile high-warning-clean on GCC... oh well

Change semantics of emitting `.h2` files: In `-p` pure mode do the existing split of phases 0 and 1 into `.h` and phase 2 into a separate `.hpp`, but in mixed mode put all phases into the `.h`
@hsutter
Copy link
Owner

hsutter commented Apr 14, 2023

Thanks @JohelEGP for that aggregate base suggestion, that's nicer -- done in the commit I just pushed

zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
Replace `store_as_base` with generated aggregate bases - better fix for hsutter#336, thanks @JohelEGP for the suggestion! This way we also don't need to obfuscate the name anywhere beyond the constructor(s), as the right member object name just enters the class's scope

If the user didn't write a constructor, provide a default constructor

If the user didn't write a 'that' constructor, suppress Cpp1's compiler-generated copying and assignment

Clean up emission of the just-mentioned generated/=deleted constructors, more naturally line up inside the class body following the indentation for other members that the original source code uses

Rename file `load.h` to `io.h` (`file.h` was another candidate), just because it has been bothering me for a year now that except for that one file all the headers were in natural alphabetical order by compilation phase... as of this commit we now have them all in alpha order and phase order: common.h -> io.h -> lex.h -> parse.h -> [*] -> sema.h -> cppfront.h

    [*] coming next here: reflect.h, which will also be in both alpha order and compilation order

Guard `out.construct` with `if constexpr` in case the type is not copy assignable and that path is never requested

Rename `cpp2::error` to `cpp2::error_entry` to quiet a new(? why?) GCC message about shadowing the former name with `parser::error`... I get why the warning is there, but this is a slightly annoying warning to have to satisfy just to compile high-warning-clean on GCC... oh well

Change semantics of emitting `.h2` files: In `-p` pure mode do the existing split of phases 0 and 1 into `.h` and phase 2 into a separate `.hpp`, but in mixed mode put all phases into the `.h`
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