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

Bugfix: Use the mouse button constant instead of 0 to avoid a compiler error on OSX #1035

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@CouleeApps
Contributor

CouleeApps commented Dec 28, 2015

Simple fix that prevents a compiler error from breaking the build in Xcode 7.0.1

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 29, 2015

Member

What error? Build fine here.

BTW, we have >> contribution guideline <<.

Member

mantognini commented Dec 29, 2015

What error? Build fine here.

BTW, we have >> contribution guideline <<.

@CouleeApps

This comment has been minimized.

Show comment
Hide comment
@CouleeApps

CouleeApps Dec 29, 2015

Contributor

The call to CGEventCreateMouseEvent was passing 0 (int) instead of kCGMouseButtonLeft (CGMouseButton), and was failing to build on the 10.11 SDK, 64 Bit. No idea why your environment works and mine doesn't, but this is a single-line replacement that just swaps the 0 for the inferred constant.

Here was the build error if you're interested:

error: no matching function for call to 'CGEventCreateMouseEvent'
CGEventRef event = CGEventCreateMouseEvent(NULL,
                   ^~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGEvent.h:57:33: note: candidate function not viable: no known conversion from 'int' to 'CGMouseButton' for 4th argument
CG_EXTERN CGEventRef __nullable CGEventCreateMouseEvent(
Contributor

CouleeApps commented Dec 29, 2015

The call to CGEventCreateMouseEvent was passing 0 (int) instead of kCGMouseButtonLeft (CGMouseButton), and was failing to build on the 10.11 SDK, 64 Bit. No idea why your environment works and mine doesn't, but this is a single-line replacement that just swaps the 0 for the inferred constant.

Here was the build error if you're interested:

error: no matching function for call to 'CGEventCreateMouseEvent'
CGEventRef event = CGEventCreateMouseEvent(NULL,
                   ^~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGEvent.h:57:33: note: candidate function not viable: no known conversion from 'int' to 'CGMouseButton' for 4th argument
CG_EXTERN CGEventRef __nullable CGEventCreateMouseEvent(

@CouleeApps CouleeApps changed the title from [OSX] Use the mouse button constant here instead of 0 to avoid a compiler error to Bugfix: Use the mouse button constant instead of 0 to avoid a compiler error on OSX Dec 29, 2015

@minirop

This comment has been minimized.

Show comment
Hide comment
@minirop

minirop Dec 30, 2015

they may have changed the definition to a strongly typed enum. Qt had the same prob

minirop commented Dec 30, 2015

they may have changed the definition to a strongly typed enum. Qt had the same prob

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 30, 2015

Member

As also mentioned in your link, simply passing kCGMouseButtonLeft is a bad idea, because it's unclear why exactly the left mouse button is chosen. You should add a comment -- there was one there, why did you remove it?

And is this backwards-compatible, i.e. was kCGMouseButtonLeft an integral constant before the enum was introduced?

Member

Bromeon commented Dec 30, 2015

As also mentioned in your link, simply passing kCGMouseButtonLeft is a bad idea, because it's unclear why exactly the left mouse button is chosen. You should add a comment -- there was one there, why did you remove it?

And is this backwards-compatible, i.e. was kCGMouseButtonLeft an integral constant before the enum was introduced?

@mantognini mantognini added bug and removed s:undecided labels Dec 30, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 30, 2015

Member

I looked into it; what's weird is that with Xcode 7.2, SDK 10.11 on 10.10, it works fine. But if I try to mimic the CF_ENUM macro that is used to define CGMouseButton in some scrap project, I get an error too. Somewhere in there something fishy is happening...

Anyway, the Qt's bug report mentioned by @minirop sheds some light on the matter. The only thing missing in this PR is the comment referred by @Bromeon. Although, I would suggest replacing the incriminated line by /* we don't care about this: */ CGMouseButton(0)); to make it even clearer.

Member

mantognini commented Dec 30, 2015

I looked into it; what's weird is that with Xcode 7.2, SDK 10.11 on 10.10, it works fine. But if I try to mimic the CF_ENUM macro that is used to define CGMouseButton in some scrap project, I get an error too. Somewhere in there something fishy is happening...

Anyway, the Qt's bug report mentioned by @minirop sheds some light on the matter. The only thing missing in this PR is the comment referred by @Bromeon. Although, I would suggest replacing the incriminated line by /* we don't care about this: */ CGMouseButton(0)); to make it even clearer.

@mantognini mantognini added this to the 2.4 milestone Dec 30, 2015

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 30, 2015

Member

Although, I would suggest replacing the incriminated line by /* we don't care about this: */ CGMouseButton(0)); to make it even clearer.

I wouldn't do this because 0 need not necessarily refer to a valid enumerator, and the function may not handle such values correctly. If it does now, it may still change in the future. Even worse, doing so could be UB.

Member

Bromeon commented Dec 30, 2015

Although, I would suggest replacing the incriminated line by /* we don't care about this: */ CGMouseButton(0)); to make it even clearer.

I wouldn't do this because 0 need not necessarily refer to a valid enumerator, and the function may not handle such values correctly. If it does now, it may still change in the future. Even worse, doing so could be UB.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 30, 2015

Member

Even though the doc specifically says this parameter is ignored in this context, on second thought you're right.

Member

mantognini commented Dec 30, 2015

Even though the doc specifically says this parameter is ignored in this context, on second thought you're right.

@CouleeApps

This comment has been minimized.

Show comment
Hide comment
@CouleeApps

CouleeApps Dec 30, 2015

Contributor

kCGMouseButtonLeft is available in OS X 10.4 and later. I added the comment back (for documentation), but this should be compatible unless you plan on supporting OS X 10.3

Contributor

CouleeApps commented Dec 30, 2015

kCGMouseButtonLeft is available in OS X 10.4 and later. I added the comment back (for documentation), but this should be compatible unless you plan on supporting OS X 10.3

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 31, 2015

Member

The constant is assured to be there, but its value might (however unlikely) be changed in later version.

Anyway, the PR is good for merging I'd say. :-)

Member

mantognini commented Dec 31, 2015

The constant is assured to be there, but its value might (however unlikely) be changed in later version.

Anyway, the PR is good for merging I'd say. :-)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Dec 31, 2015

Member

I'd squash the two commits and write a better commit message than "Use the constant here..." and "...this variable..." 😛

Member

Bromeon commented Dec 31, 2015

I'd squash the two commits and write a better commit message than "Use the constant here..." and "...this variable..." 😛

@CouleeApps

This comment has been minimized.

Show comment
Hide comment
@CouleeApps

CouleeApps Dec 31, 2015

Contributor

Alright, should be one commit now, and the title is more specific.

Contributor

CouleeApps commented Dec 31, 2015

Alright, should be one commit now, and the title is more specific.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jan 1, 2016

Member

👍

Member

Bromeon commented Jan 1, 2016

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 3, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jan 3, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r self-assigned this Jan 15, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 19, 2016

Member

Merged in fe9b9c0

Member

eXpl0it3r commented Feb 19, 2016

Merged in fe9b9c0

@eXpl0it3r eXpl0it3r closed this Feb 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment