search

lbryio / lbrycrd

The blockchain that provides the digital content namespace for the LBRY protocol
https://lbry.com
MIT License
2.57k stars 178 forks source link

Fix newest boost and GCC warnings #148

Closed bvbfan closed 6 years ago

bvbfan commented 6 years ago

Signed-off-by: Anthony Fieroni bvbfan@abv.bg

lbrynaut commented 6 years ago

@bvbfan You're right, the required boost is a bit dated. Rather than change that, I think the slight performance hit of using boost::shared_ptr instead is the way to go instead of unique_ptr.

bvbfan commented 6 years ago

Furthermore i will return auto_ptr, since we can plan to switch for C++11, hopefully soon :) I compile project with C++11 and did not realize that is not C++11 :) Then auto_ptr just become unique and it's done.

lbrynaut commented 6 years ago

@bvbfan Right, not C++11. The claimtrie could be completely re-factored and cleaned up when we support C++11, but alas, it remains where it is for now. It's on the roadmap (at least we've discussed it before).

bvbfan commented 6 years ago

Yes, i made a huge refactor of CClaimTrie not only to move db in separate class / file even CClaimCache should be refactored and renamed, as discussed before.

lbrynaut commented 6 years ago

@bvbfan I'm aware. I meant a C++11 mechanical language re-factor not an organizational/architectural re-factor.

bvbfan commented 6 years ago

Yes, i hope after architectural refactor, step to C++11 refactor will be easiest, i hope, second time :)

bvbfan commented 6 years ago

You can merge this, if you does not have any other remarks. :)

lbrynaut commented 6 years ago

It looks good to me. I'm waiting on any input from @kaykurokawa, otherwise may merge later this afternoon.

kaykurokawa commented 6 years ago

I'm a bit hesitant to merge this. If we are modifying Bitcoin Core code like this, than there will be a lot of fixes, and we will have a bad time trying to merge from upstream which we will eventually have to do.

I checked through some of these changes in the most recent bitcoin code base and it does indeed seem to be fixed in the same way you did. Maybe the better way to fix it would be to cherry-pick these commits from Bitcoin Core (not saying you have to do this for this PR, but in the future, I would check to see if you aren't redoing what has already been done).

It is important though to fix warnings for our quality of life purposes. I think we should get a process in place to say that "hey this commit can be ignored once we merge upstream", this way we can just ignore them and merging will be easier. Maybe something like putting in the commit comments, "NOT FOR MERGE UPSTREAM" ? We can just search and remove these commits. Or maybe these types of changes all go in to the same branch and get merged from that branch?

bvbfan commented 6 years ago

I don't notice that they changed upstream, it can be cherry-pick from upstream Core for master, then ignored when merge it from my specific branch, it is OK?

lbrynaut commented 6 years ago

@kaykurokawa Good point. I say we punt on this until we formally merge from upstream. It's something that I plan to start in the next month or two, so I think it's better to not make things potentially harder later at this point.

tzarebczan commented 6 years ago

What's the status of this pr? @bvbfan

lbrynaut commented 6 years ago

@tzarebczan I'm of the opinion that it was a good exercise, but should be closed unmerged for now. Upstream changes being pulled in are on the roadmap, which should resolve a lot of this.

lyoshenka commented 6 years ago

If we cherry-pick commits from upstream, I don't think we need to mark them in any special way. Git should be able to handle this, and either way Kay told me its likely the way we get up to date is by starting with the current Core code and merging in our stuff. So maybe there's no harm in merging this?

@kaykurokawa correct me if I'm wrong.

bvbfan commented 6 years ago

So this should be closed i think