Skip to content
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

Merged
merged 2 commits into from
Oct 29, 2015
Merged

Do not export internal unsafe_encode_char() #55

merged 2 commits into from
Oct 29, 2015

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
@@ -10,7 +10,8 @@ INSTALL=install
CFLAGS ?= -O2
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