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

Modify the definition of SDImageFormat to extern to avoid creating multiple global variables #3699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tujinqiu
Copy link

I found that several formats about SDImageFormat are static variables defined in a header file. When the program is compiled and linked, any .m file that imports or indirectly imports this header will create a static variable inside the file. This results in an increase in useless binary size. In fact, only need to declare these variables as extern and assign values ​​inside a certain .m file, so that there will only be one copy of these variables in the entire binary.

@dreampiggy
Copy link
Contributor

dreampiggy commented Apr 15, 2024

Looks good for me.


By the way, I want to talk about this SDImageFormat as well

This shit design (using a enum) is from the history SDWebImage 2.x, and changed into typedef NSUInteger in 4.x, but not been refactored during the 5.x release.

The most stupid is that this is actually not Extensible, though even it can be. The code using NSUInteger in whatever version is bad, because the int value based can be conflict, it's not decentralized, unlike our coder API design. (See https://github.com/SDWebImage/SDWebImage/wiki/Coder-Plugin-List, this ugly int based typedef is centralized)

In SD 6.0, I'll totally remove this bad design, use class instead.

// In SDWebImage Core repo
@interface SDImageFormat : NSObject
@property (readonly, nonnull) NSString *UTI;
@property (readonly, nonnull) NSString *preferredFilenameExtension;
@property (readonly, nonnull) NSString *preferredMIMEType;
@end

// Each coder repo, like JPEGXL
@interface SDImageFormat (JPEGXL)
@property (class, readonly) SDImageFormat *JPEGXL;
@end 

// WebP repo
@interface SDImageFormat (WebP)
@property (class, readonly) SDImageFormat *WebP;
@end 

// APNG
@interface SDImageFormat (APNG)
@property (class, readonly) SDImageFormat *APNG;
@end 

return [self.class canEncodeToFormat:SDImageFormatWebP];
default:
return NO;
if (format == SDImageFormatWebP) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler can do optimization based on enum cases number, these changes is no needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

The compiler reports an error when declare these variables as extern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is this cause API break (assume user use code like this in switch case?)

If so, why not totally break and use class declarations instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered...In 5.c

This static declaration is written by me to allows "switch case" works

This code is indeed, so seems this is not possible in 5.x

@dreampiggy
Copy link
Contributor

So, at least, we need to fix the 10+ individual coder repo as well (they use static declaration)

Can we wait for the total break changes to use class based SDImageForamt instead ?

@tujinqiu
Copy link
Author

So, at least, we need to fix the 10+ individual coder repo as well (they use static declaration)

Can we wait for the total break changes to use class based SDImageForamt instead ?

This is just a small optimization, looking forward to version 6.0

@dreampiggy dreampiggy added this to the 6.0.0 milestone Apr 16, 2024
@dreampiggy dreampiggy force-pushed the master branch 3 times, most recently from 095d3ad to 78fe228 Compare May 8, 2024 08:49
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

Successfully merging this pull request may close these issues.

None yet

2 participants