- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.2k
fix(ios): reduced TitaniumKit build warnings & fixed TiBlob crash #14298
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
base: main
Are you sure you want to change the base?
Conversation
| JSContext *context = JSContext.currentContext; | ||
| JSValueRef *exception; | ||
| JSObjectRef arrayBuffer = JSObjectMakeArrayBufferWithBytesNoCopy(context.JSGlobalContextRef, arrayBytes, len, jsArrayBufferFreeDeallocator, nil, exception); | ||
| JSObjectRef arrayBuffer = JSObjectMakeArrayBufferWithBytesNoCopy(context.JSGlobalContextRef, arrayBytes, len, jsArrayBufferFreeDeallocator, nil, NULL); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why passing NULL here instead of initializing the exception to NULL and passing the reference like you did below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because exception is not evaluated resp. used further here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should though - any exception handling can be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how?
Handling it like in arrayBuffer via KrollPromise then it would make toArrayBuffer and arrayBuffer redundant.
Handling it with the JavaScriptCore API (I am not familiar with it), ChatGPT suggests the following:
    if (exception) {
        JSStringRef exceptionStr = JSValueToStringCopy(context.JSGlobalContextRef, exception, NULL);
        NSString *message = (__bridge_transfer NSString *)JSStringCopyCFString(kCFAllocatorDefault, exceptionStr);
        JSStringRelease(exceptionStr);
        // e.g. handle message
        
        free(arrayBytes);
        return nil;
    }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, we are there even both then, if the only difference is the exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, there is still a difference – a different context may be used (JSContext.currentContext vs. [self currentContext]).
Related to #14290. Edit: Also fixes #14306.
Description
Removed over 40 warnings during TitaniumKit build process:
+resourceBasedURL:baseURL:iniphone/TitaniumKit/TitaniumKit/Sources/API/ScriptModule.m-JSValueInContext:iniphone/TitaniumKit/TitaniumKit/Sources/API/KrollModule.m-applyProperties:iniphone/TitaniumKit/TitaniumKit/Sources/API/TiProxy.hJSValueRef *vs.JSValueRef _Null_unspecifiediniphone/TitaniumKit/TitaniumKit/Sources/API/TiBlob.mJSValueRef, which is already a pointer (Edit: crash relevant)JSValueRefJSValueRef *CAShapeLayer *vs.CALayer *iniphone/TitaniumKit/TitaniumKit/Sources/API/TiUIView.mTiHostiniphone/TitaniumKit/TitaniumKit/Sources/API/ScriptModule.mNSUncaughtExceptionHandler *iniphone/TitaniumKit/TitaniumKit/Sources/API/TiExceptionHandler.mTiBlobTypeSystemImageiniphone/TitaniumKit/TitaniumKit/Sources/API/TiBlob.mdefaultcase