Storage types fixed

[pfeatherstone]

Problem:

• Local storage didn't work
• Move constructors of erased type were being called in copy constructors of poly

Solution:

• Storage types handle memory manually. So longer need void_ptr
• Deleted copy semantics are converted to exceptions in polys
• Deleted move semantics are converted to exceptions in polys

Issue: #31

Reviewers: @krzysztof-jusiak

[pfeatherstone]

@krzysztof-jusiak I've also added sbo_storage and shared_storage

[krzysztof-jusiak]

nice, can we add some more tests please to ensure that functionality won't get removed in the future

[pfeatherstone]

Yep no problem

[pfeatherstone]

I added some tests. I don't think the tests are as complete as they should be. But we could add more in future PRs

[pfeatherstone]

@krzysztof-jusiak OK, i've added a few more tests, specifically that test value semantics more closely depending on the storage type. If everything passes, I would be keen for you to review. Thanks

[krzysztof-jusiak]

Fantastic, thanks

[pfeatherstone]

I've added bug fixes and tests for self-assignment :)

[pfeatherstone]

Hmm, with the fixes and noexcept-correctness, the library generates a lot more assembly. So the whole "better inlining" argument goes out the window. Maybe there are some optimizations possible

[pfeatherstone]

Hmm, with the fixes and noexcept-correctness, the library generates a lot more assembly. So the whole "better inlining" argument goes out the window. Maybe there are some optimizations possible

Compiler explorer experiments hint that's because I've removed loads of noexcept specifiers. I think we should, otherwise it's a lie. The underlying types may well throw during construction or assignment.

[pfeatherstone]

Like, te::call shouldn't have a noexcept either. Otherwise a program will terminate even if the culprit is in a try-catch statement.

[pfeatherstone]

I don't understand what Codacy is chatting about

[pfeatherstone]

I've tried so many different compilers, and it always works fine. So I really don't know what Codacy is talking about. It doesn't make sense to have an explicit templated constructor with a universal reference. I'm not even sure that would be valid C++. @krzysztof-jusiak any ideas?

[pfeatherstone]

I give up fighting Codacy. I would love it if someone showed me what I'm doing wrong. The code builds everywhere (everywhere i've been bothered to try)

[hgkjshegfskef]

@pfeatherstone Perhaps I don't understand the problem, but is there any reason you don't want to make the poly constructor explicit, as the tool suggests? From a cold view, right now the tool complains that poly constructor takes a single argument but the constructor is not marked explicit.

[pfeatherstone]

I have a few more ideas for another PR. For example, the storage policies should have additional policies that dictate whether or not they should be copyable, moveable, noexcept copyable or noexcept moveable. At the moment, all of them are copyable and moveable. If the underlying erased type is not copyable say, then the copy constructor or copy assign operator of the poly throws. Which means we can't mark the copy constructor or copy assignment operators as noexcept. Similar thing for move semantics. Or depending on the policy, we may want to delete copy or move semantics. these types of things will chip away lines of assembly. The default behavior before was to mark everything as noexcept which I think is incorrect because your program will always terminate. i don't think that's desirable.

[pfeatherstone]

@krzysztof-jusiak Do you mind reviewing when you get the chance?

[krzysztof-jusiak]

@pfeatherstone very nice, thank you :clap: I think it just need rebasing and it's good to go :+1:

[pfeatherstone]

@krzysztof-jusiak Sorry, I'm not a git expert. Do you mind giving some instructions? This branch is ahead of boost-ext/te anyway so not sure what rebasing would do. Do you mean squashing commits?

[hgkjshegfskef]

@pfeatherstone see https://github.com/boost-ext/te/issues/19 regarding compilation errors with GCC

[pfeatherstone]

@hgkjshegfskef Thanks. Now i know what name to put to that mappings_size() thing. I was calling it ADL hacking magic. So don't know what to do then. Surely we want this to work with gcc, it is after all the most popular compiler. I guess reflection in C++23 (or whenever it comes) will solve all of this.

[hgkjshegfskef]

@pfeatherstone if you understand how that "ADL magic" works (I unfortunately do not), perhaps you can borrow the implementation from Anton Polukhin's Boost.PFR, which works under c++17 and even c++14, which, as far as I understand, needs to do something similar. That issue is 4 years old, and nobody suggested a patch yet, so good luck :)

[pfeatherstone]

I think Boost.PFR uses something different. It has to do with aggregate initialization.

[pfeatherstone]

I think we need @krzysztof-jusiak to lay out his opinion on this. Making this work on gcc without changing the API would be nice. I'm not a fan of the Dyno interface which requires you to define several things. This is super clean. But if it can't be done with gcc then it can't be done. Sy Brand did something really great using the old metaclasses proposal. I'm sure something equivalent with reflection will be possible soon and all problems will go away. In the mean time, we make this a Clang only library.

[krzysztof-jusiak]

hmm, it seems that the soution has been broken for g++8+ borefe these changes either way. I think we can proceeed with the PR as it's a great improvement and figure out whether there is a valuable wknd or g++8+. I do remember seeing a workable solution somewhere at some point. Will try to dig it in.

[krzysztof-jusiak]

Regardin the MR conficts

Merging via command line
If you do not want to use the merge button or an automatic merge cannot be performed, you can perform a manual merge on the command line. However, the following steps are not applicable if the base branch is protected.

https://github.com/pfeatherstone/te.git
Step 1: From your project repository, check out a new branch and test the changes.

git checkout -b pfeatherstone-master master
git pull https://github.com/pfeatherstone/te.git master
Step 2: Merge the changes and update on GitHub.

git checkout master
git merge --no-ff pfeatherstone-master
git push origin master

[pfeatherstone]

I don't quite understand what you want me to do with the rebase. Can't we just do a squash and merge? I made my changes on master. When I run your commands I just get messages like "Already up-to-date". I'm really sorry if I'm being dumb.

[pfeatherstone]

hmm, it seems that the soution has been broken for g++8+ borefe these changes either way. I think we can proceeed with the PR as it's a great improvement and figure out whether there is a valuable wknd or g++8+. I do remember seeing a workable solution somewhere at some point. Will try to dig it in.

It also doesn't work for certain versions of Clang, for example 11.0.0, 10.0.1, 9.0.1, 8.0.1. https://godbolt.org/z/TdTKaah18 Basically, some of the compilers really don't like templated type erasure.

[krzysztof-jusiak]

merged, thanks @pfeatherstone, @hgkjshegfskef

[pfeatherstone]

Awesome. Now we just need it to work on gcc and other versions of clang.

[pfeatherstone]

hmm, it seems that the soution has been broken for g++8+ borefe these changes either way. I think we can proceeed with the PR as it's a great improvement and figure out whether there is a valuable wknd or g++8+. I do remember seeing a workable solution somewhere at some point. Will try to dig it in.

@krzysztof-jusiak Is this relevant to fixing the implementation such that it works with g++ and some version of clang?