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

1584 simplifications #120

Merged
merged 11 commits into from
Apr 23, 2019
Merged

Conversation

pmconrad
Copy link

@pmconrad pmconrad commented Apr 4, 2019

1st round of simplifications. Removed lots of stuff. The result is still compatible with current core.

Sorry about the mess in the 1st commit. I had it separated into many nice and clean commits, then destroyed my .git folder. :-/ Note to self: never use find | xargs sed -i in top-level project dir.

Highlights in 1st commit:

  • Replaced fc::any with boost::any
  • Replaced fc::min with std::min
  • Replaced fc::move with std::move
  • Replaced fc::string with std::string (core still uses fc::string so can't remove the typedef yet)
  • Replaced fc::aligned<> with alignas
  • Replaced fc::remove_reference with std::remove_reference
  • Replaced fc_swap with std::swap

The last change is to remove the openssl-based ECC implementation, which is slow and disabled by default. See discussion in bitshares/bitshares-core#1584

More to follow, but that will require changes in core as well, so I wanted to get this in now.

Copy link

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

Small items for Windows and Mac. The increase in tcp_socket.hpp line 54 is interesting. You may know more about it than I do. As I see there was a similar problem in file_mapping.hpp.

Another consideration: CMakeModules/FindBOOST.cmake is broken on macOS and Windows. Would you consider removing it as part of your cleanup? Things build fine without it on those platforms, and probably Linux as well (haven't tried yet).

include/fc/io/iostream.hpp Show resolved Hide resolved
include/fc/network/tcp_socket.hpp Show resolved Hide resolved
include/fc/network/http/connection.hpp Show resolved Hide resolved
@pmconrad
Copy link
Author

pmconrad commented Apr 9, 2019

I think the reason for the problems with fwd<...> is the replacement of fc::aligned with alignas.
Apparently a union (which is what backed fc::aligned) can be larger than its largest element when alignment comes into play. This means the constants have always been too small, only it was never noticed because the backing union was large enough.

@pmconrad
Copy link
Author

pmconrad commented Apr 9, 2019

Tried to remove FindBoost, but ran into a linker error concerning libpthread. See discussion in #121 .

@jmjatlanta
Copy link

Tried to remove FindBoost, but ran into a linker error concerning libpthread. See discussion in #121 .

I guess my dream will have to wait, then. :-) Now compiles on macOS and Windows. I am now working on a more thorough review.

Copy link

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

Excellent job. Tested on Ubuntu 19.04 / Boost 1.67 / BitShares develop branch. Tests in FC and core/chain_test run with no errors detected. Code looks clean.

@jmjatlanta jmjatlanta dismissed their stale review April 22, 2019 14:20

Concerns addressed

Copy link

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

Excellent job. Tested on Ubuntu 19.04 / Boost 1.67 / BitShares develop branch. Tests in FC and core/chain_test run with no errors detected. Code looks clean.

@jmjatlanta
Copy link

Question for my own education: The uncommenting of deque pack and unpack... Everything compiles if this remained commented out. Would the raw pack/unpack have taken over?

@pmconrad
Copy link
Author

They were previously in fc/container/deque_fwd.hpp, which I deleted. Didn't test compilation with removing them completely.

@pmconrad pmconrad merged commit 7f93f26 into bitshares:master Apr 23, 2019
@pmconrad pmconrad deleted the 1584_simplification branch April 23, 2019 13:54
MatusKysel added a commit to abilitiinc/AbilitiX that referenced this pull request May 13, 2019
@abitmore abitmore added this to the core-release-3.2.0 milestone Aug 21, 2019
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