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

To "this" or not to "this"... #57

Open
dot-asm opened this issue Feb 8, 2021 · 5 comments
Open

To "this" or not to "this"... #57

dot-asm opened this issue Feb 8, 2021 · 5 comments

Comments

@dot-asm
Copy link
Collaborator

dot-asm commented Feb 8, 2021

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?
@mratsim
Copy link
Contributor

mratsim commented Feb 9, 2021

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.

@dot-asm
Copy link
Collaborator Author

dot-asm commented Feb 10, 2021

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
Copy link
Collaborator Author

dot-asm commented Feb 10, 2021

"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 wordwork. But one can make a case for a = sum(b, c)...

@Nashatyrev
Copy link

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
Copy link

And thanks for considering this issue 👍

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

No branches or pull requests

3 participants