search

emersion / go-imap

📥 An IMAP library for clients and servers
MIT License
2.1k stars 296 forks source link

v2: The int and Pointer #649

Closed taoso closed 2 weeks ago

taoso commented 2 weeks ago

Hi, I am digging the source of go-imap v2 and found the following issues.

In the StatusData, some fields are defined as integer pointer:

https://github.com/emersion/go-imap/blob/3e14ef3cd2b7de584d2f27881d8e017e4f794802/status.go#L20-L33

In golang, it's common to use the pointer to save optional variable. If the pointer is nil, there is no value for this variable. And if you need them, you have to assure the pointer is non-nil.

However, when go-imap use these pointers in StatusData, it does no nil pointer check. Instead, there is already criteria of options. If some option is true, it's corresponding field of StatusData must be none nil

https://github.com/emersion/go-imap/blob/3e14ef3cd2b7de584d2f27881d8e017e4f794802/imapserver/status.go#L53-L80

So why we need these pointers? It seems lateral variable is enough.

If we use non-pointer integer and need to represent some variable is not existed, we can assign the -1 to it.

In golang, the pointer should be used as necessary~

The second question is why we need to distinct value between int32 or int64? As far as I know, the IMAP is pure text protocol. All integers will be represented in the decimal form. Maybe the int is enough, and only strconv.{Itoa, Atoi} are needed.

emersion commented 2 weeks ago

Please don't use the issue tracker to ask questions.

taoso commented 2 weeks ago

Hi @emersion sorry for disturbing. I have read the issue template and it says don't ask question using issue.

However, this issue is not a question about how to use this package, it's about the design and implementation. It's not an easy task to discuss such problem in the IRC. And maybe it make sense to use the issue to trace the discussion process. And the IRC is not a good tool to make asynchronous discussion across different time zones ;-(

Sorry again. But what's the right place to continue this discuss? Or do you think it make any sense to continue this topic?

Thanks.

emersion commented 2 weeks ago

I maintain this software in my free time on a best-effort basis. I may or may not have time to answer design questions on IRC. Sorry, this is as good as it's going to get - I'm overwhelmed.

taoso commented 2 weeks ago

@emersion Thanks for your contribution 👍

BTW, this issue is not out of my curiosity, but from my experience of Golang and the desire to try to participate the contribution journey.

foxcpp commented 2 days ago

The second question is why we need to distinct value between int32 or int64? As far as I know, the IMAP is pure text protocol. All integers will be represented in the decimal form. Maybe the int is enough, and only strconv.{Itoa, Atoi} are needed.

IMAP specification defines specific bit size for some of the fields (uid, uidvalidity, condstore timestamps). To ensure go-imap is able to represent all valid values irregardless of platform-specific int size, int32 or int64 is used.

foxcpp commented 2 days ago

As for the main question, options is client-specified and represents what is client interested in, while nil/non-nil is specified by the server to represent missing values which is valid for some protocol fields. Therefore, I think there are indeed missing nil checks in the library code.