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

Code Style #24

Open
leptos-null opened this issue Feb 27, 2020 · 3 comments
Open

Code Style #24

leptos-null opened this issue Feb 27, 2020 · 3 comments

Comments

@leptos-null
Copy link
Member

Per README:

Follow the existing coding style

There are many different styles throughout the headers, even ignoring vendor headers (e.g. openssl).

I believe there should be a more unified, and explicit style.

Examples of different existing styles

Style discussions are sometimes criticized, because developers have different opinions on how code should be formatted, however I mention these three because I've seen issues arise in due to these in particular.

Availability attributions are important, especially for projects such as this one, where private system APIs are sometimes documented. Deciding on an attribution system allows developers to contribute more easily and effectively.

External declarations must be accurate when using C++, to avoid linking errors. Theos used to create ObjC++ source files by default. A unified extern marking system would avoid issues such as FSSwitchState.h missing a C++ linker hint.

Property attributes are very strongly a style choice, but I mention it due to #21 changing these property declarations. It could be bothersome, if someone felt compelled to change them back, or if new changes are made with no property trailing space, and someone else goes to change those.

@kirb
Copy link
Member

kirb commented Feb 28, 2020

Thanks so much for caring about this.

The reason for some of the inconsistencies is due to “eras” so to speak, e.g. the availability system was overhauled only a few years ago and this is when API_AVAILABLE and co. were introduced. I don’t really end up changing things like availability macros; partly this is since I want headers for old things to keep working on old toolchains/SDKs. API_AVAILABLE is awkward to backport to clang releases that don’t use it due to the more complex syntax than the preprocessor really supports. The macros definitely could be added everywhere (that’d be amazingly useful) but it’d also be the most pain in the butt part of building these headers.

“Proper” code style issues like @property(…) (no space) or getter = something (too many spaces) should definitely be cleaned up; I would have done my best to police this but it’s likely some have slipped through.

__{BEGIN,END}_DECLS could go either way - we could keep doing that to ensure it’s safe to use whether C or C++, or we could switch to FOUNDATION_EXTERN. Following Apple’s pattern is probably the most ideal.

I’ve erred on not changing code style of 3rd-party headers such as the libpackageinfo changes seen in the linked PR, because we want to easily keep them in sync when they change. Changing code style would create merge conflicts, possibly causing bits and pieces of upstream changes to be lost by accident in cleaning up the conflicts. So I’m preferring timeliness on merging updated sets of headers over code style there. I think we could make an exception for Flipswitch here to fix its lack of C++ compatibility. Ideally should send that upstream as well so we’re only patching it this once.

GraphicsServices and a few others came from earlier collections of headers such as KennyTM’s, and I wasn’t so sure whether it’s worth cleaning them up or not. I think it is worth it since we know now that the upstream repos are unlikely to be updated again - this repo is probably the canonical place to get these headers from now. So we may as well bring them to our standards.

If you’re up for cleaning up code style based on what I’ve discussed here then that would be a big help 🙂

(p.s. @PoomSmart, sorry I completely forgot to review your pull request!)

@uroboro
Copy link
Member

uroboro commented Jun 23, 2020

Should we set up clang-format so we don’t have to worry about manually modifying headers? I know that brings up the discussion of the specifics of the style we pick but it’d save us from the work of applying it

@leptos-null
Copy link
Member Author

I’m supportive of a clang-format.

My recommendation would be to use the WebKit (or another) default, and modify the rules as needed to match the current style

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