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

Fix compiler warning when compling NSFontAssetRequest.m in libs-gui #334

Closed
wants to merge 1 commit into from

Conversation

qmfrederik
Copy link
Contributor

Cast the result type of CALL_NON_NULL_BLOCK to void to satisfy the ? operator.

If not, compiling on clang on msys2/ucrt64 fails with this error:

NSFontAssetRequest.m:48:3: error: incompatible operand types ('int' and 'id')
    48 |   CALL_BLOCK(completionHandler, error);
        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/tools/msys64/ucrt64/include/GNUstepBase/GSBlocks.h:110:53: note: expanded from macro 'CALL_BLOCK'
    110 | #define CALL_BLOCK(block, args...) ((NULL != block) ? CALL_NON_NULL_BLOCK(block, args) : nil)
        |                                                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~   ~~~
1 error generated.

@qmfrederik qmfrederik requested a review from rfm as a code owner October 9, 2023 20:31
@rfm
Copy link
Contributor

rfm commented Oct 10, 2023

I don't think that can be correct, because the result of the call can be assigned (which is impossible for void).
Of course this is designed to be used where we don't have real blocks, and to support a subset of the possible return types.
So presumably we want to cast to 'id' rather than 'void' (or perhaps cast both sides to 'void*')?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Cast the result type of CALL_NON_NULL_BLOCK to void to satisfy the ? operator.

```
NSFontAssetRequest.m:48:3: error: incompatible operand types ('int' and 'id')
    48 |   CALL_BLOCK(completionHandler, error);
        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/tools/msys64/ucrt64/include/GNUstepBase/GSBlocks.h:110:53: note: expanded from macro 'CALL_BLOCK'
    110 | #define CALL_BLOCK(block, args...) ((NULL != block) ? CALL_NON_NULL_BLOCK(block, args) : nil)
        |                                                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~   ~~~
1 error generated.
```
Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

@rfm
Copy link
Contributor

rfm commented Oct 12, 2023

Oops, failed to notice that the compiler objects. I wonder why?

@rfm
Copy link
Contributor

rfm commented Oct 12, 2023

I guess the problem is that the block may return a value or may not return a value at all.
If so, the idea of having a simple macro to cope with all cases may be fundamentally broken.
Possibly we need one macro to call a block which doesn't return a value, and a separate macro to call a block which does return a value.
Something like:

#define CALL_BLOCK(block, args...) ({if (NULL != block) CALL_NON_NULL_BLOCK(block, args);})
#define CALL_BLOCK_RET(block, rettype, args...) ((NULL != block) ? (rettype)CALL_NON_NULL_BLOCK(block, args) : (rettype)0)

Does that seem reasonable?
Of course existing code using the macro may need to be changed.

@rfm rfm self-requested a review October 12, 2023 14:38
rfm added a commit that referenced this pull request Nov 14, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@rfm
Copy link
Contributor

rfm commented Nov 14, 2023

In th absence of feedback, I went ahead with that last proposal.

@rfm rfm closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants