Skip to content

[SUGGESTION] change to unique and shared in cpp2 namespace #685

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

Open
farmerpiki opened this issue Sep 16, 2023 · 12 comments
Open

[SUGGESTION] change to unique and shared in cpp2 namespace #685

farmerpiki opened this issue Sep 16, 2023 · 12 comments

Comments

@farmerpiki
Copy link
Contributor

They cause issues when being used with c++ modules ... yes, I know we're not supposed to talk about them yet... but they are supposed to be supported as first class citizens eventually.

Solutions include:

  • renaming it as a namespace and removing the const on the functions and changing how they are accessed
  • naming the structs instead and declaring the functions inside them static
  • leaving everything dynamic and making them extern and available in a separately compiled file
  • actually creating a module with their definitions
  • leave a single name for each way to do things... right now for unique.new we have 3 names: new, cpp2_new and unique.new, maybe a single way for each would be simpler (new and shared_new?)
  • have a way to annotate new(@shared new) ... the annotation may allow us to later choose which arena to allocate gc.new from

I'm just trying to pick on what the best path forward is, maybe it's too soon for the discusion and I understand that but some of these solutions change syntax a bit

@JohelEGP

This comment was marked as resolved.

@farmerpiki
Copy link
Contributor Author

it's clang 16 with libstdc++ and headers from gcc 12.3 ... and I have the change from static to inline for nonesuch imported, should I try to produce minimal code to reproduce the problem?

@farmerpiki
Copy link
Contributor Author

farmerpiki commented Sep 17, 2023

I tried to minimize the example as much as possible still needs 2 modules, but at least I got it relatively short, I left both the cpp2 files and generated files, also the build.log file and added a build.sh to see the commands I used to build this.
https://github.com/farmerpiki/cpp2-issue

the relevant part from the build.log file
In module 'personal' imported from /home/g-zu/projects/proofoffailure/public.pure.cpp:9:
/home/g-zu/projects/cppfront/include/cpp2util.h:519:3: error: definition with same mangled name '_ZN4cpp26uniqueE' as another definition
} unique;
^
/home/g-zu/projects/cppfront/include/cpp2util.h:519:3: note: previous definition is here
} unique;
^

P.S. I don't know, this might very well be a compiler issue with current modules, but since we plan to support them and the issue is present in one of the major compilers...

@JohelEGP
Copy link
Contributor

shared isn't mentioned in the output.
So the reason for me not noticing this issue is because I used neither of the names unique or shared.
You did use unique, which is why you noticed.

@farmerpiki
Copy link
Contributor Author

I didn't use shared but I imagine the same thing would happen since it's declared the same

@farmerpiki
Copy link
Contributor Author

farmerpiki commented Sep 19, 2023

This still happens with inline!?
It also happens with shared, I checked

@hsutter hsutter reopened this Sep 19, 2023
@hsutter
Copy link
Owner

hsutter commented Sep 19, 2023

OK, reopening... my impression was that changing them to inline would resolve the issue but evidently not, thanks for the information. It seemed like inline was an improvement anyway so I went ahead and did it, and I'm open to other improvements.

I'm not yet working on enabling full support for modules, until modules support appears and stabilizes in the major compilers. But I'm still happy to entertain suggestions for changes that would help this issue, especially if they would be an improvement anyway like inline on those static objects.

Thanks!

@farmerpiki
Copy link
Contributor Author

Any idea which would be closer to an acceptable solution? I tried and the namespace version seems to work... though it involves one extra character in total...
I can try as many options as you wish in order to make it work if you give me ideas.
Willing to put in the work for this, unique pointers are especially dear to my heart and I want to have this work, trying to write a personal project with as much as possible pure cpp2 syntax.

@JohelEGP

This comment was marked as resolved.

@farmerpiki
Copy link
Contributor Author

Oh, so nothing changed cause they're in the cpp2 namespace?
Would there be a chance we can include a way to declare the destructor type in the syntax of new? For the cases when we use libraries which allocate their own pointers and have special functionality to free them.

@JohelEGP
Copy link
Contributor

Seems like make_unique doesn't permit specifying the deleter.

@farmerpiki
Copy link
Contributor Author

would you be opposed to just moving the stuff from instantiated structs into functions

template
[[nodiscard]] auto cpp2_new(auto &&...args) -> std::unique_ptr {
return std::make_unique(CPP2_FORWARD(args)...);
}

template
[[nodiscard]] auto cpp2_shared_new(auto &&...args) -> std::unique_ptr {
return std::make_shared(CPP2_FORWARD(args)...);
}

these just work, without inline, static or other tricks
also we could have them as unique_new and shared_new instead of unique.new and shared.new and everything else would work the same I guess.

I realize this is a breaking change, I could also move them to a separate file just for your patches JohelEGP and build them as a separate module.

zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
Also make `unique`, `shared`, and `nonesuch` be `inline`, closes hsutter#685.

And finally fix `token::to_string`'s default to never decorate, all callers passed `true` anyway except the one use in `parse_tree_printer::start(token)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants