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

refactor template instantiation #1516

Merged
merged 6 commits into from
Feb 5, 2019
Merged

refactor template instantiation #1516

merged 6 commits into from
Feb 5, 2019

Conversation

jmjatlanta
Copy link
Contributor

This is another way to fix #1504 that works for macOS, and probably others.

Instead of including fc/smart_ref_impl.hpp, this fix permits the template to generate the specific copy constructor that is needed.

I am unable to test on the originally reported platform (openSUSE 15.04) , as I do not have that environment.

@jmjatlanta jmjatlanta requested a review from pmconrad January 7, 2019 16:07
@jmjatlanta
Copy link
Contributor Author

jmjatlanta commented Jan 7, 2019

Note: I threw in another fix that is not related to the above, but does allow for compilation on macOS. I thought it too small for a separate PR, but it is in a separate commit (regarding size_t to uint64_t) should others think it better to separate them.

@pmconrad
Copy link
Contributor

pmconrad commented Jan 7, 2019

Travis build fails with "ambiguous template specialization"

@@ -210,7 +210,7 @@ namespace graphene { namespace db {
// private
static const size_t MAX_HOLE = 100;
static const size_t _mask = ((1 << chunkbits) - 1);
size_t next = 0;
uint64_t next = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message should indicate that the "variable" is the next sequence counter used by database indices.

@jmjatlanta
Copy link
Contributor Author

Travis build fails with "ambiguous template specialization"

Hmmm. It compiles fine in Release mode, but I can replicate the failure in Debug mode. Interesting I will investigate.

@jmjatlanta
Copy link
Contributor Author

Travis build fails with "ambiguous template specialization"

I have looked through the code, and unless I am missing something big, this smart_ref stuff is of little use. It is only used in fee_schedule.

Of course, because of its use there, there needs to be code to serialize and de-serialize it. Plus the actual template definitions. And you will need to include the smart_ref header everywhere you use fee_schedule.

It seems like a whole lot of hoops to do what I think a smart pointer could do.

Then there are problems with the template on mac. It seems to have been causing problems on that platform for a while, as a static "tmp" of smart_ref<fee_schedule> was added a few years ago to allow mac to compile it.

In an attempt to fix recent mac issues, adding a simple forward-declare causes problems with the overloaded ! operator.

All of the above makes me think about ripping it out. But two issues appear:

  1. Am I not seeing something that smart_ref does that some kind of pointer does not?
  2. If it were to be ripped out, what would need to be done to make things backward-compatible with serialized objects?

Looking for input/opinions.

Note: I can fix the mac overloaded ! by doing a specialization. It is an easy fix, but the above questions remain.

@pmconrad
Copy link
Contributor

pmconrad commented Jan 8, 2019

Hm, this looks somewhat useless indeed.

Serialization of smart_ref<T> is identical to serialization of T, except for one level of nesting in raw serialization. Possible replacement would be std::unique_ptr, with the caveat that smart_ref cannot contain a null pointer, plus there is no raw serialization for std::unique_ptr yet AFAICS.

@jmjatlanta jmjatlanta mentioned this pull request Jan 31, 2019
3 tasks
@pmconrad pmconrad self-assigned this Feb 5, 2019
Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed previously, let's merge this as a quick fix and aim for a proper solution later on.

@jmjatlanta jmjatlanta merged commit f1e4480 into develop Feb 5, 2019
@jmjatlanta jmjatlanta deleted the jmj_1504 branch February 5, 2019 14:33
@jmjatlanta
Copy link
Contributor Author

The real fix will be done as part of #1548. This fix just eliminates the need to put a header in a bunch of files. Compiles fine on Ubuntu 18.10, macOS 10.13. Windows build seems to be going fine, will update here if there are any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants