search

theos / logos

Preprocessor that simplifies Objective-C hooking.
https://theos.dev/docs/logos
Other
205 stars 34 forks source link

Simplify base generator's `staticDeclarations` #106

Closed L1ghtmann closed 1 year ago

L1ghtmann commented 1 year ago

What does this implement/fix? Explain your changes.

Simplifies the macro conditional used to set various Logos constants

Does this close any currently open issues?

No

Any relevant logs, error output, etc?

Any other comments?

N/A

Where has this been tested?

Operating System: …

Platform: …

Target Platform: …

Toolchain Version: …

SDK Version: …

L1ghtmann commented 1 year ago

Given that this is only for use with clang (see __clang__) I don't think that'll be a problem

Edit: Whoops, I see your point...that doesn't seem to matter as the syntax is invalid for gcc. Seems like we don't allow gcc to be used for Darwin targets but I wonder if older clang's had similar behavior and that's why it was discrete like that.

leptos-null commented 1 year ago

I'm not following.

Are you saying this file will only be processed by clang? if so, why do we need the if defined(__clang__) check at all?

Or are you saying that since the if defined(__clang__) check is there, we don't need to worry about gcc picking up the rest of the code? If so, this analysis is not correct. Prior to this PR, this code would compile under gcc and with this PR, the code does not compile under gcc

Edit: all good. no, if clang had similar behavior, it wouldn't matter (since it's still clang, and the first part would evaluate to true either way). Darwin used to compile under both gcc and clang, so I think that's why it's like that. For compatibly reasons, I'd prefer not to change it unnecessarily.

L1ghtmann commented 1 year ago

I'm saying that we only allow gcc for non-Darwin targets and to my knowledge Logos isn't used for any of those. I realized my error in judgment after I read the rest of your comment (whoops!) in that it doesn't pass the initial gcc preprocess so the check being there for clang is irrelevant.

leptos-null commented 1 year ago

ha, we're both editing and adding new comments to quickly. No worries, and my apologies as well.

Given this, I think we should either remove the if defined(__clang__) or keep the current code. Per my second edit, I'd lean towards keeping the current code.

Do you have any particular motivation for shortening the generated code?

L1ghtmann commented 1 year ago

I just happened to stumble across it when testing something else and thought it odd that it was discrete, so, nope, all good to keep as-is. Thanks for checking gcc!