blst icon indicating copy to clipboard operation
blst copied to clipboard

To "this" or not to "this"...

Open dot-asm opened this issue 4 years ago • 5 comments

Quoting @Nashatyrev in #54.

  • (codestyle nit not directly related to this PR) there are methods like class P1 { P1 add(P1 a); } which modify and return this instance. From my perspective they could be erroneously interpreted as that a new instance is returned and this instance is left unmodified. Not sure about common C++ patterns, but Java dev would likely misinterpret this. Does it make sense to return void from these methods to avoid such misuse?

dot-asm avatar Feb 08 '21 09:02 dot-asm

Apparently it seems to be a pattern in Go as well: https://github.com/ConsenSys/gurvy/blob/e350ead/bls381/g1.go#L72-L77

In Java, this is used for the "Builder Pattern" as well so I think Java devs would actually be familiar with this.

mratsim avatar Feb 09 '21 10:02 mratsim

Incidentally we've got a somewhat similar question even in Python context. Yes, favouring the "Builder Pattern" is the reason for returning the same object, yet one can make a case for returning a new object of the same type instead. And in environments where programmers are excused from managing memory it works out just as well. Well, not really "as well," because it will be less efficient. After all, you'll "spew" a number of unreferenced objects in the process, which means that it will trigger garbage collector more often, or burden each invocation more. I for one would argue that it's formally inappropriate, and therefore would suggest those who ask to return new objects to "take one for the team." Because it would be a nightmare for the rest of us:-) However! One can also argue that it's not like the "Builder Pattern" is universally justified, and it would be appropriate to "void" selected methods. add and dbl are probably perfect examples/first candidates. For example, one can use the Pattern to multiply a point by 42 as following:

px42 = p.dup().dbl().dbl().add(p).dbl().dbl().add(p).dbl()

But it really has more "academic" value than practical, doesn't it? As for other methods, I'd be reluctant. Yes, not even mult... Primarily because blst.G1().mult(n) is ultimately practical.

To summarize. Shall we limit ourselves to "void"-ing add and dbl methods?

dot-asm avatar Feb 10 '21 21:02 dot-asm

"take one for the team."

In essence it boils down to following simple rule. Whenever there is assignment, first method ought to be dup(). For example, equivalent of a = b + c would be a = b.dup().add(c), and equivalent of a += b would be a.add(b), no assignment.

Just in case, the reason for why there is no a = add(b,c) is because swig messes it up for some languages, so that no add-s ~word~work. But one can make a case for a = sum(b, c)...

dot-asm avatar Feb 10 '21 21:02 dot-asm

In Java, this is used for the "Builder Pattern" as well so I think Java devs would actually be familiar with this.

Yeah, but basically builder instance is created in-place and doesn't leak from the minimal scope. It also usually has build() call at the end 😄 Anyways feel free to stick to any solution and we will just adopt.

Nashatyrev avatar Feb 12 '21 09:02 Nashatyrev

And thanks for considering this issue 👍

Nashatyrev avatar Feb 12 '21 09:02 Nashatyrev