Skip to content

Fix various warnings #125

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 3, 2016
Merged

Fix various warnings #125

merged 3 commits into from
Mar 3, 2016

Conversation

nalimilan
Copy link
Contributor

Cf. #117.

EdSchouten
EdSchouten reviewed Feb 28, 2016
View reviewed changes
src/fpmath.h
#if !(defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && defined(__ORDER_BIG_ENDIAN__))

#elif defined(__GLIBC__)
#if defined(__GLIBC__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of leaving all of this code there, would it be worth just throwing it away and seeing what breaks? It may be the case that Windows is the only platform that doesn't define these. All the other systems use a compiler that provides support for these definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that I have no way to check other compilers, I'd rather keep these as it's not much code and it's pretty clear. Do you have any reference about what other compilers support? The only thing I have found is https://sourceforge.net/p/predef/wiki/Endianness/, which doesn't seem to say so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking on behalf of FreeBSD, the __FreeBSD__ block can safely be removed.

@nalimilan
Copy link
Contributor Author

Test failure happens since f7a18b5.

EdSchouten
EdSchouten reviewed Feb 28, 2016
View reviewed changes
src/fpmath.h Outdated
#if _BYTE_ORDER == _LITTLE_ENDIAN
#if _IEEE_WORD_ORDER == _LITTLE_ENDIAN
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
#if _IEEE_WORD_ORDER == __ORDER_LITTLE_ENDIAN__
Copy link
Contributor

Choose a reason for hiding this comment

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

At least GCC provides __FLOAT_WORD_ORDER__ for this purpose. Would it make sense to also use this name directly instead of using _IEEE_WORD_ORDER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Will fix.

@nalimilan nalimilan mentioned this pull request Feb 28, 2016
Closed
EdSchouten
EdSchouten reviewed Feb 28, 2016
View reviewed changes
src/fpmath.h Outdated
#endif /* __BYTE_ORDER__, __ORDER_LITTLE_ENDIAN__ and __ORDER_BIG_ENDIAN__ */

#ifndef __FLOAT_WORD_ORDER__
#define __FLOAT_WORD_ORDER__ __BYTE_ORDER__
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a space between #define and __FLOAT_WORD_ORDER__, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, the file originally contained a tab. Fixed.

@nalimilan nalimilan force-pushed the nl/includes branch 5 times, most recently from 75cdbc4 to f6063b5 Compare February 28, 2016 16:04
@nalimilan
Copy link
Contributor Author

I've pushed a new commit removing all unused macros from bsd_cdefs.h, and I must say I quite like it (about -1000 LOC). Even without renaming them, this shuts down the warnings on Linux.

BTW, it seems to me that in math_private.h, #ifdef __GNUCLIKE_ASM is always false, as only bsd_cdefs.h defines it, and it's not included AFAICT. Am I right?

@ViralBShah
Copy link
Member

I have reverted the -fno-builtins for now to get CI running again, and will turn it on more systematically later.

@ViralBShah
Copy link
Member

If we can rebase this on master, we may be able to get a green.

nalimilan added 3 commits March 2, 2016 21:21
@nalimilan
Fix misleading indentation (spotted by GCC 6)
949c530
@nalimilan
Avoid redefining endianness macros with some gcc versions
b5ca785 
_LITTLE_ENDIAN and _BIG_ENDIAN are built-in on some platforms/versions.
Better use __ORDER_LITTLE_ENDIAN__, __ORDER_BIG_ENDIAN__ and __BYTE_ORDER__,
which are standard for gcc and clang, and define them when they are missing.
Also remove the special-case for FreeBSD, which is apprently not needed.
@nalimilan
Cleanup bsd_cdefs.h, merge i387 and amd64 duplicate files
2c3f3b5 
Most macros were not actually used. This gets rid of warnings when
building on Linux.
@nalimilan
Copy link
Contributor Author

Cool, I've just rebased.

Note that I've kept bsd_cdefs.h and cdefs-compat.h separate even if they are much smaller now, because merging them triggers errors I wasn't able to debug. Since you know the codebase much better, you might be able to clean this a bit more.

ViralBShah added a commit that referenced this pull request Mar 3, 2016
@ViralBShah
Merge pull request #125 from JuliaLang/nl/includes
3851036 
Fix various warnings
@ViralBShah ViralBShah merged commit 3851036 into master Mar 3, 2016
@simonbyrne
Copy link
Member

Thanks, that fixes the warnings on Power.

@simonbyrne simonbyrne deleted the nl/includes branch March 3, 2016 09:28
@ViralBShah
Copy link
Member

Cc @edelsohn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nalimilan @ViralBShah @simonbyrne @yuyichao @EdSchouten @tkelman