dropbox / djinni

A tool for generating cross-language type declarations and interface bindings.
Apache License 2.0
2.88k stars 487 forks source link

Rename destroy() to __destroy() #365

Closed sheldonneuberger closed 6 years ago

sheldonneuberger commented 6 years ago

This prevents compilation errors when someone creates a djinni interface with their own method named "destroy()".

j4cbo commented 6 years ago

Names with two leading underscores are reserved, so _destroy would be better (or, better yet, _djinni_destroy)

artwyman commented 6 years ago

We have a use of a djinni_private prefix for a similar purpose here: https://github.com/dropbox/djinni/blob/master/support-lib/objc/DJIObjcWrapperCache%2BPrivate.h#L91

That being said, I don't have any serious objection to _djinni_destroy. Only one underscore, and including the word djinni should make it plenty safe against conflicts, and consistency between the ObjC++ and JNI helpers isn't a big deal.

sheldonneuberger commented 6 years ago

Tests pass. I changed it to _djinni_private_destroy so that it's more consistent with objc; doesn't hurt to have that. I prefer the underscore in front to make it more clear that it's special and private, but if someone objects we can also drop that leading underscore.

xianwen commented 6 years ago

Hi, @sheldonneuberger: I noticed you haven't signed the CLA yet, could you please sign it here: https://opensource.dropbox.com/cla/ Thanks a lot!

sheldonneuberger commented 6 years ago

Signed.

On Mon, May 14, 2018, 1:29 PM Xianwen Chen notifications@github.com wrote:

Hi, @sheldonneuberger https://github.com/sheldonneuberger: I noticed you haven't signed the CLA yet, could you please sign it here: https://opensource.dropbox.com/cla/ Thanks a lot!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dropbox/djinni/pull/365#issuecomment-388952312, or mute the thread https://github.com/notifications/unsubscribe-auth/AC42aN1_AiAJX6qFOC5gtYAroMhQcoITks5tyelBgaJpZM4T7727 .

sheldonneuberger commented 6 years ago

@xianwen possible to merge this today?