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

Support logger without variable argument list #276

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@school510587
Contributor

school510587 commented Jul 22, 2017

libchewing only accepts logger callback with variable argument list so far. However, it seems difficult to implement such callback using other language (eg. Python). This prevents direct access of log messages. Here, the PR implements a new built-in logger and a new API to remove the restriction.

  • It is cleaner to use vasprintf. However, vasprintf is not in C standard, and additional implementation will be needed to pass MSVC compilation.

school510587 added some commits Jul 22, 2017

Accept logger without variable argument list
Data4NoVarArgListLogger keeps the real logger and data obtained from the
caller. Logger4NoVarArgList collects message properly and forwards it to
the real logger.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 22, 2017

Coverage Status

Coverage decreased (-0.4%) to 90.212% when pulling 447b5fc on school510587:no_var_arg_list_logger into e469f5b on chewing:master.

coveralls commented Jul 22, 2017

Coverage Status

Coverage decreased (-0.4%) to 90.212% when pulling 447b5fc on school510587:no_var_arg_list_logger into e469f5b on chewing:master.

@school510587 school510587 changed the title from Support logger without variable argument listNo var arg list logger to Support logger without variable argument list Jul 23, 2017

Show outdated Hide outdated include/chewingio.h
@school510587

This comment has been minimized.

Show comment
Hide comment
@school510587

school510587 Aug 9, 2017

Contributor
Contributor

school510587 commented Aug 9, 2017

@jserv

This comment has been minimized.

Show comment
Hide comment
@jserv

jserv Aug 10, 2017

Member

Cc. @czchen

The functionality of chewing_new2 is to provide another way to initialize Chewing engine. However, the function proposed by @school510587 is a different story, IMHO. I would suggest explicit naming.

Member

jserv commented Aug 10, 2017

Cc. @czchen

The functionality of chewing_new2 is to provide another way to initialize Chewing engine. However, the function proposed by @school510587 is a different story, IMHO. I would suggest explicit naming.

@@ -545,7 +545,7 @@ CHEWING_API int chewing_get_phoneSeqLen(const ChewingContext *ctx);
CHEWING_API void chewing_set_logger(ChewingContext *ctx,
void (*logger) (void *data, int level, const char *fmt, ...), void *data);
CHEWING_API void chewing_set_logger2(ChewingContext *ctx,
CHEWING_API void chewing_set_logger_without_varglist(ChewingContext *ctx,

This comment has been minimized.

@jserv

jserv Aug 11, 2017

Member

The suffix without_varglist is too long. I would expected to revise the use of API instead.

@jserv

jserv Aug 11, 2017

Member

The suffix without_varglist is too long. I would expected to revise the use of API instead.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 11, 2017

Coverage Status

Coverage decreased (-0.4%) to 90.215% when pulling 4ba342c on school510587:no_var_arg_list_logger into e469f5b on chewing:master.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.4%) to 90.215% when pulling 4ba342c on school510587:no_var_arg_list_logger into e469f5b on chewing:master.

@jserv

This comment has been minimized.

Show comment
Hide comment
@jserv

jserv Aug 11, 2017

Member

How about using Swig to resolve the convention?
Reference: http://www.swig.org/Doc1.3/Varargs.html

Member

jserv commented Aug 11, 2017

How about using Swig to resolve the convention?
Reference: http://www.swig.org/Doc1.3/Varargs.html

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