dropbox / djinni

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

Make "const auto" where variable not being modified and ask about (void). #426

Closed shahzadlone closed 5 years ago

shahzadlone commented 5 years ago

Hello, first of all I would like to thank the folks who have contributed to this code base, I have never explored a more beautifully written C++ code base!!

Here is a small change that kind of was triggering my coding OCD :,). I don't think size gets modified anywhere so I made it const (please correct me if I am wrong).

Moreover, if time permits I would really appreciate if a few of my curiosities can be answered (for learning purposes):

on line 466 and 467 of djinni_support.cpp we have the following:

    (void)implWStringToUTF16<2>;
    (void)implWStringToUTF16<4>;

Could we not change the (void) to one of the following solutions ? or is there something wrong with them? and why is (void) more useful over them.

artwyman commented 5 years ago

Heh, I appreciate your obsessive use of const. My life would be better if more of the languages I use included const as a construct.

Thanks for signing the CLA in advance too.

Regarding marking of unused things: I think any of the approaches you suggest would be effective at hiding the warnings. I think all of your suggestions would likely also force instantiation of the template for the two specific values, which some alternatives might not (e.g. annotations on the definition site). The annotation requires specific compiler support. The unused template probably requires a header if you're not going to define it all over place. The static casts should be entirely equivalent. Mostly I think (void)foo is just a recognizable pattern going all the way back to C so it's what I think people tend to reach for.

shahzadlone commented 5 years ago

Thanks for your informative reply:

The unused template probably requires a header if you're not going to define it all over place.

Here did you mean the ignore template?

artwyman commented 5 years ago

Right, I typed the wrong thing.

shahzadlone commented 5 years ago

No worries! Thanks for the merge.

Also is there a way to get more involved in the open source contribution of Djinni?

artwyman commented 5 years ago

PRs are always welcome, but realistically the project is not very active these days. There are active users, including Dropbox, but not much change in the needed use cases, so not much investment in new features. I'd encourage you to join the Slack community mentioned in the README for discussion with other users/developers, though.