-
Notifications
You must be signed in to change notification settings - Fork 140
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
Do not export internal unsafe_encode_char() #55
Conversation
Strange, the tests with |
I added the compiler flag
|
Yes, we've had some persistent problems with the |
The appveyor test seems to have bitrotted as well, grrr.... |
Do not export internal unsafe_encode_char()
Just to make sure that my change does not introduce a silent regression to the tests: Was there any particular reason why the tests were using |
@@ -10,7 +10,8 @@ INSTALL=install | |||
CFLAGS ?= -O2 | |||
PICFLAG = -fPIC | |||
C99FLAG = -std=c99 | |||
UCFLAGS = $(CFLAGS) $(PICFLAG) $(C99FLAG) -DUTF8PROC_EXPORTS | |||
WCFLAGS = -Wall -Wmissing-prototypes -pedantic |
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.
Is -Wmissing-prototypes
not included in -Wall
? If it isn't, why not? MSVC understands -Wall
but not -Wmissing-prototypes
, and currently my hacky method of building Julia with MSVC goes through makefiles rather than cmake for some deps.
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.
It's not included in -Wall
for gcc.
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.
If it causes problems with MSVC, I would just omit this warning.
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.
Better suggestion: move -Wmissing-prototypes
to CFLAGS
in .travis.yml
.
The flag serves a purpose, to warn when internal functions are accidentally exported.
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 like that plan, as long as people will skim the travis logs once in a while
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.
See .travis.yml
, the flag -Werror
ensures that Travis will fail on warnings.
The 2.0 release and subsequent #77 proved that warnings will be ignored with certainty unless fatal. 😉
This commit removes a spurious external symbol for the internal function
unsafe_encode_char()
, which I noticed while preparing a package of the utf8proc library for Debian.