Make it compile nicely on 32 bit ARM platforms with GCC#19
Open
azaparov wants to merge 1 commit intofbbdev:masterfrom
Open
Make it compile nicely on 32 bit ARM platforms with GCC#19azaparov wants to merge 1 commit intofbbdev:masterfrom
azaparov wants to merge 1 commit intofbbdev:masterfrom
Conversation
Owner
|
Thank you! Could you please elaborate a bit why is this necessary? In theory Is this not true? Which compiler errors are you seeing? |
Author
|
Yes it is 32 bit on arm. But all max/min functions in the code compare point coordinates with constants that look like “1L” (long int) so type matching breaks and compiler throws errors.
Perhaps using “L” suffix is not necessary? I doubt there is terminal in real life that will require long int precision :)
Regards.
… On Feb 1, 2019, at 4:28 AM, Fabio Massaioli ***@***.***> wrote:
Thank you! Could you please elaborate a bit why is this necessary? In theory std::ptrdiff_t should be a pointer-sized signed type on any platform, so I expected it to be the same as long int also on 32-bit ARM.
Is this not true? Which compiler errors are you seeing?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Author
|
Perhaps cleaner fix will be to explicitly cast all constants to point coordinate type. I did not go this route with time constraints I had.
… On Feb 1, 2019, at 4:28 AM, Fabio Massaioli ***@***.***> wrote:
Thank you! Could you please elaborate a bit why is this necessary? In theory std::ptrdiff_t should be a pointer-sized signed type on any platform, so I expected it to be the same as long int also on 32-bit ARM.
Is this not true? Which compiler errors are you seeing?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Owner
|
Oh I see... Ironically I introduced all those L prefixes precisely to fix type matching 😄 Yeah the right solution should be explicit casting like I won't merge right now, but I'm keeping this PR open while I have a look at the code |
Author
|
Sure thing :) Cool library btw!
… On Feb 1, 2019, at 9:23 AM, Fabio Massaioli ***@***.***> wrote:
Oh I see... Ironically I introduced all those L prefixes precisely to fix type matching 😄
Yeah the right solution should be explicit casting like Coord(1). I can take some time to make the change myself if you don't have time. Or maybe I could improve min/max templates and make this unnecessary. Let's see.
I won't merge right now, but I'm keeping this PR open while I have a look at the code
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make it compile nicely on 32 bit ARM platform