Skip to content

Conversation

@petercolberg
Copy link
Contributor

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.

@petercolberg
Copy link
Contributor Author

Strange, the tests with make check are passing, but the make data target is failing.

@petercolberg
Copy link
Contributor Author

I added the compiler flag -Wmissing-prototypes that warns in case of exported internal functions:

cc -O2 -Wall -Wmissing-prototypes -fPIC -std=c99 -pedantic -DUTF8PROC_EXPORTS -c -o utf8proc.o utf8proc.c
utf8proc.c:191:18: warning: no previous prototype for ‘unsafe_encode_char’ [-Wmissing-prototypes]
 utf8proc_ssize_t unsafe_encode_char(utf8proc_int32_t uc, utf8proc_uint8_t *dst) {
                  ^

@stevengj
Copy link
Member

Yes, we've had some persistent problems with the make data check because the ruby script seems to be producing non-deterministic tables, probably due to differences in hashing algorithms in different ruby versions or on different systems; we haven't tracked it down yet.

@stevengj
Copy link
Member

The appveyor test seems to have bitrotted as well, grrr....

stevengj added a commit that referenced this pull request Oct 29, 2015
Do not export internal unsafe_encode_char()
@stevengj stevengj merged commit e52c8c4 into JuliaStrings:master Oct 29, 2015
@petercolberg
Copy link
Contributor Author

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 unsafe_encode_char() instead of utf8proc_encode_char()?

@petercolberg petercolberg mentioned this pull request Nov 1, 2015
PICFLAG = -fPIC
C99FLAG = -std=c99
UCFLAGS = $(CFLAGS) $(PICFLAG) $(C99FLAG) -DUTF8PROC_EXPORTS
WCFLAGS = -Wall -Wmissing-prototypes -pedantic
Copy link
Contributor

@tkelman tkelman Jul 14, 2016

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.

Copy link
Member

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.

Copy link
Member

@stevengj stevengj Jul 15, 2016

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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. 😉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants