Avoid using boost::optional in PassField() #17

pull vasild wants to merge 1 commits into bitcoin-core:master from vasild:remove-boost-from-PassField changing 1 files +16 −12

vasild commented at 7:56 PM on January 15, 2020: contributor

Refactor PassField() to an equivalent one that does not use boost::optional.

Avoid using boost::optional in PassField()

Refactor PassField() to an equivalent one that does not use
boost::optional.

a616312ed8

vasild requested review from ryanofsky on Jan 15, 2020
vasild commented at 8:21 PM on January 15, 2020: contributor

@ryanofsky as a followup to https://github.com/chaincodelabs/libmultiprocess/pull/16 I crudely stripped down absl::optional to about 4k lines of code (:-O). With finer stripping maybe it could go down to below 1000 lines, but it does not look good - what if a bug is introduced due to the stripping? Also, changes from upstream would be very tedious to merge and it would be one black box inside the source code. Its better to require the user to install absl externally.

While playing with it the patch in this PR materialized - we could remove some more boost without adding a boost::optional replacement.
ryanofsky commented at 8:39 PM on January 15, 2020: collaborator

This seems like a good approach, and I can merge this PR after a little testing. I agree that replacing optional isn't good if it's going to add a lot of new code. I was thinking a small optional class with basic emplace/get functionality could be implemented in something like ~100 lines, but if this isn't the case it's probably better to avoid adding a new optional class. And maybe it's better to avoid using optional altogether, since most of the changes you've made so far do seem like simplifications.
in include/mp/proxy-types.h:963 in a616312ed8
```
 965 | -            Emplace<decltype(param)>(param));
 966 | -        if (!param) param.emplace();
 967 | +
 968 | +    if (!input.want()) {
 969 | +        fn.invoke(server_context, std::forward<Args>(args)..., nullptr);
 970 | +        server_context.call_context.getResults();
```
ryanofsky commented at 10:24 PM on January 16, 2020:

Should be safe to drop this line. Can be done in a followup though since keeping it makes the refactoring more obvious.

vasild commented at 4:08 PM on January 17, 2020:

https://github.com/chaincodelabs/libmultiprocess/pull/18

Cheerz!
ryanofsky approved
ryanofsky commented at 10:25 PM on January 16, 2020: collaborator

Code review ACK a616312ed8edc16431bfc33b55740fc0ffd17658
ryanofsky referenced this in commit d3388da2c0 on Jan 16, 2020
ryanofsky merged this on Jan 16, 2020
ryanofsky closed this on Jan 16, 2020
vasild deleted the branch on Jan 17, 2020
bitcoin-core locked this on Jun 25, 2025

Contributors

vasild

ryanofsky

Linked (view graph)

#115 MSAN Error during connection shutdown #162 macos "Broken pipe" exceptions during tests #219 SIGABRT in ~ProxyClientBase with #29409 and rust client