Reduce boilerplate in applySteps: #4710
Conversation
| default: | ||
| assert(false); | ||
| return XRPAmount{0}; | ||
| return with_txn_type(tx.getTxnType(), [&]<typename T>() { |
There was a problem hiding this comment.
Why do the view and tx parameters not need to be captured in the lambda?
There was a problem hiding this comment.
The [&] will default capture all the needed variables by ref. So view and tx are both captured. This could have been written as [&view, &tx] but there's no reason to call attention to what variables are captured.
There was a problem hiding this comment.
On a related note, will the const-ness of these refs be preserved through the lambda? I remember watching a lecture about this a long time ago, but I'm not able to discern it now.
There was a problem hiding this comment.
i found some solutions for the const question here: https://stackoverflow.com/questions/3772867/lambda-capture-as-const-reference
|
@seelabs - do you you have an opinion on whether this should go in 2.0 (with XChainBridge and lots of other changes), or wait for 2024? I hope this is a safe refactor, but you never know ;) |
|
@mvadari What do you mean by plug-in code? Is it a different PR? |
It will be in a future PR - it's still WIP. |
|
@intelliot I don't think we need to delay this. I think we can merge this into the next release. |
seelabs
force-pushed
the
refactor-apply-steps
branch
from
31e1f05
to
fcba9b4
Compare
|
Force pushed to resync with my local branch. I also disabled maintainers from pushing to this branch. It's easier for me to track things if I'm the only one who pushes. |
|
note: do not merge until after PR author applies |
|
@gregtatcam Nice suggestion! I agree the "requires" is nicer (it's not part of vocabulary yet so reached for |
Sure, will do. |
There was a problem hiding this comment.
I freakin' love this. Approved as-is.
...But I am going to throw you a curve ball. What do you think of getting rid of the exception type, and using an UnknownTxn placholder transactor instead? To me, the big advantage is that it puts all the error conditions into one place (the class) instead of spread around the catch clauses.
Here's the change: seelabs/rippled@refactor-apply-steps...ximinez:rippled:sd-4710-apply-steps
When a new transactor is added, there are several places in applySteps that need to be modified. This patch refactors the code so only one function needs to be modified.
|
@ximinez (I really wish I could respond to top level comment in github). That's an interesting design! It is nice that it gets rid of the try/catch. My biggest objection is it's a non-standard way to handle errors. It's relatively easy to look at the current code and understand what's going on - exceptions mean errors, there's a try/catch block...yeah, yeah, yeah we've seen it a million times. Your design is codes the error as concrete class that returns the correct error code. It's really clever! I kinda like it! But it's also a way of handling errors in a way we don't usually do in rippled. I'm a little hesitant to introduce another error handling mechanism. Having said all that, I do like what you did. What do you think about getting this in now and maybe do your patch as a later PR? (I'll admit another minor concern is another round of reviews, and I'm trying to get this merged; But that shouldn't be a strong criteria). |
seelabs
force-pushed
the
refactor-apply-steps
branch
from
c887c0b
to
6475511
Compare
|
Squashed and rebased onto the latest develop |
seelabs
added
the
Passed
High Level Overview of Change
When a new transactor is added, there are several places in applySteps that need to be modified. This patch refactors the code so only one function needs to be modified.
Type of Change