rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Implement changes for Issue 3417

Open ckeshava opened this issue 2 years ago • 5 comments

If merged, this code will:

  • Implement the simplified condition evaluation
  • removes the complex bitwise and(&) operator
  • Implement the second proposed solution in Nik Bougalis's comment - https://github.com/XRPLF/rippled/issues/3417#issuecomment-635116233
  • I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.

ckeshava avatar Jul 15 '22 21:07 ckeshava

@nbougalis I have a question about the choice of integers inside the Condition enum. Does it necessarily have to be powers of 2? With respect to the operations in this file, it suffices to choose different integer values right?

I understand that the choice of even/odd values is important because we do bit-wise and operator.

ckeshava avatar Jul 15 '22 21:07 ckeshava

They have to be powers of two if you want to be able to set and test the bits individually.

For example if you define:

enum { A=1, B=2, C=3};

and you do:

uint32_t x = C;

the value of x will be 0x3 (the two lower bits set), so you would have both (x & A) != 0 and (x & B) != 0

if you use powers of two the bits can be set and tested individually. I used to use enums like:

enum {
   A = 1 << 0,
   B = 1 << 1,
   C = 1 << 2,
   ...
   };

greg7mdp avatar Jul 15 '22 22:07 greg7mdp

They have to be powers of two if you want to be able to set and test the bits individually.

For example if you define:

enum { A=1, B=2, C=3};

and you do:

uint32_t x = C;

the value of x will be 0x3 (the two lower bits set), so you would have both (x & A) != 0 and (x & B) != 0

if you use powers of two the bits can be set and tested individually. I used to use enums like:

enum {
   A = 1 << 0,
   B = 1 << 1,
   C = 1 << 2,
   ...
   };

okay got it, thanks for the explanation! I would individually check all the conditions and that is quite verbose, this is concise 👍

ckeshava avatar Jul 15 '22 23:07 ckeshava

You can use a sequence 1,2,3,4,5,... if all the conditions are exclusive from one another. But if they are flags that can be set independently, then it has to be separate bits to be workable.

greg7mdp avatar Jul 16 '22 01:07 greg7mdp

You can use a sequence 1,2,3,4,5,... if all the conditions are exclusive from one another. But if they are flags that can be set independently, then it has to be separate bits to be workable.

Okay, got it 👍

ckeshava avatar Jul 19 '22 21:07 ckeshava

@ckeshava would you like to pick this up?

intelliot avatar Apr 24 '23 18:04 intelliot

Hello, I have completed the code changes. The unit tests are passing locally, I'm not sure why the CI tests are failing on the ubuntu machine.

ckeshava avatar Apr 25 '23 23:04 ckeshava

(Holding this for 1.12 unless someone requests otherwise)

intelliot avatar Jun 09 '23 23:06 intelliot

This has CI failures which need to be resolved (or explained). @ckeshava or @thejohnfreeman - could you have a look?

intelliot avatar Jun 26 '23 19:06 intelliot

Please merge develop.

thejohnfreeman avatar Jun 26 '23 19:06 thejohnfreeman

OK, I've rebased to the develop branch, thanks for the tip!

ckeshava avatar Jun 26 '23 21:06 ckeshava

@HowardHinnant because there have been changes pushed after your earlier approval, could you do a quick re-review?

intelliot avatar Jun 28 '23 00:06 intelliot