MailCore / mailcore2

MailCore 2 provide a simple and asynchronous API to work with e-mail protocols IMAP, POP and SMTP. The API has been redesigned from ground up.
Other
2.62k stars 627 forks source link

Remove MCOIndexSet ? #365

Closed tiennou closed 11 years ago

tiennou commented 11 years ago

Right now, I'm trying to wrap parts of your API, and I can't use NSIndexSet because MC asserts if I do that (/src/objc/utils/NSObject+MCO.mm:128: assert 0). So I'll need to implement conversion methods somewhere.

But, there's already an NSIndexSet in Foundation, so I'm wondering about what's the exact purpose of `MCOIndexSet. Maybe I'm missing something but to me it looks like API duplication.

I can take care or tearing out MCOIndexSet and implement the required stuff in a category on NSIndexSet if you think that's a good idea. I'll likely do the same of MCORange in that case.

mronge commented 11 years ago

MCOIndexSet uses 64 bit numbers while NSIndexSet does not.

However, in practice I wonder if we will ever run into this limitation?

tiennou commented 11 years ago

Okay, that's what I was wondering, but isn't NSUInteger 64bit then ? That's what NSIndexSet uses...

ocrickard commented 11 years ago

NSUInteger's definition is:

#if __LP64__ || (TARGET_OS_EMBEDDED && !TARGET_OS_IPHONE) || TARGET_OS_WIN32 || NS_BUILD_32_LIKE_64
typedef long NSInteger;
typedef unsigned long NSUInteger;
#else
typedef int NSInteger;
typedef unsigned int NSUInteger;
#endif

As you can see, NSUInteger is of different size depending on platform, and mailcore is supposed to be cross-platform...

That being said, I've never seen a uid/sequence number larger than 32-bit int, but of course @dinhviethoa is the expert on this.

tiennou commented 11 years ago

Right, I was approaching that more like "32 bit OS X is essentially deprecated so NSUInteger = long". There's another point I want to make : this is Objective-C. This heavily restricts what "multi-platform" means here (OS X, iOS, and GNUStep/OpenSTEP). So I understand there's a low-level mailcore::indexset multi-platform object, it's just that this MCOIndexSet wrapper feels out-of-place. I mean, everyone who like me will want to shield themselves from the API will hit this problem (no conversion to/from NSIndexSet, and will have to build conversion methods as categories on either MCOISet or NSISet. I'm perfectly happy to keep it, but either there should be some code to ease the conversion, or remove it altogether and wrap NSIndexSet like the other Foundation classes are.

ocrickard commented 11 years ago

Excellent points, Agreed. In the past, I used NSIndexSet and mapped into MCOIndexSet. It is a bit of a headache constantly converting.

EDIT: removed an off-hand comment that was unintentionally inflammatory.

dinhvh commented 11 years ago

I think I added it because I had some use case where I wanted to store 64-bits integers and NSIndexSet did not allow it. (Maybe it's been fixed?) Though, it wouldn't work on iOS since iOS, for now, is running on 32-bits hardware.

One real issue I can think about though is that I use UINT64_MAX as an IMAP *. I don't think I can do that in NSIndexSet since 2^32-1 would be confusing. It could mean * or the actual value.

Let me know if you have some suggestion.

That said, I'll definitively work on making conversion between NSIndexSet and MCOIndexSet easier.

dinhvh commented 11 years ago

Filed #370.