The error macros are problematic

[richfitz]

I get a number of compiler warnings from the softfailure, hardfailure and ERROR macros (and function macros are pretty gross in general IMO).

There are a few approaches to fixing these, but all will involve some moderate refactoring of the code. You may have thoughts on how best to do this, or be looking at other bits of refactoring that would overlap.

With clang (Apple LLVM version 7.0.0 (clang-700.0.72)) and with -Wall -Wextra -pedantic -Wno-unused-parameter I see

In file included from liblsoda/src/lsoda.c:72:
liblsoda/src/lsoda_internal.h:18:106: warning: token pasting of ',' and
      __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ...ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE_...
                                                      ^
liblsoda/src/lsoda.c:174:3: warning: C99 forbids conditional expressions with
      only one void side [-Wpedantic]
                ERROR("[lsoda] neq = %d is less than 1\n", ctx->neq);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
liblsoda/src/lsoda_internal.h:18:37: note: expanded from macro 'ERROR'
#define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strd...
                                    ^~~~~~~~~~~~~~~~
liblsoda/src/lsoda_internal.h:18:106: warning: token pasting of ',' and
      __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ...ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE_...
                                                      ^
liblsoda/src/lsoda.c:190:5: warning: C99 forbids conditional expressions with
      only one void side [-Wpedantic]
  ...ERROR("[lsoda] rtol = %g is less than 0.\n", rtoli);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
liblsoda/src/lsoda_internal.h:18:37: note: expanded from macro 'ERROR'
#define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strd...
                                    ^~~~~~~~~~~~~~~~
liblsoda/src/lsoda_internal.h:18:106: warning: token pasting of ',' and
      __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
  ...ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE_...
                                                      ^
liblsoda/src/lsoda.c:193:5: warning: C99 forbids conditional expressions with
      only one void side [-Wpedantic]
  ...ERROR("[lsoda] atol = %g is less than 0.\n", atoli);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
liblsoda/src/lsoda_internal.h:18:37: note: expanded from macro 'ERROR'
#define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strd...
                                    ^~~~~~~~~~~~~~~~

(and so on)

With gcc (same flags)

lsoda.c: In function ‘check_opt’:
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:174:3: note: in expansion of macro ‘ERROR’
   ERROR("[lsoda] neq = %d is less than 1\n", ctx->neq);
   ^
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:190:5: note: in expansion of macro ‘ERROR’
     ERROR("[lsoda] rtol = %g is less than 0.\n", rtoli);
     ^
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:193:5: note: in expansion of macro ‘ERROR’
     ERROR("[lsoda] atol = %g is less than 0.\n", atoli);
     ^

[snip]

lsoda.c: In function ‘lsoda’:
lsoda.c:510:76: warning: ISO C99 requires rest arguments to be used [enabled by default]
    hardfailure("[lsoda] illegal common block did you call lsoda_prepare?\n");
                                                                            ^
lsoda.c:510:76: warning: ISO C99 requires rest arguments to be used [enabled by default]
In file included from lsoda.c:72:0:
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:105:2: note: in expansion of macro ‘ERROR’
  ERROR(fmt, ## __VA_ARGS__); \
  ^
lsoda.c:510:4: note: in expansion of macro ‘hardfailure’
    hardfailure("[lsoda] illegal common block did you call lsoda_prepare?\n");
    ^
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:105:2: note: in expansion of macro ‘ERROR’
  ERROR(fmt, ## __VA_ARGS__); \
  ^
lsoda.c:535:6: note: in expansion of macro ‘hardfailure’
      hardfailure("[lsoda] tout = %g behind t = %g. integration direction is given by %g\n",
      ^
lsoda.c:569:66: warning: ISO C99 requires rest arguments to be used [enabled by default]
      hardfailure("[lsoda] itask = 4 or 5 and tcrit behind tout\n");
                                                                  ^
lsoda.c:569:66: warning: ISO C99 requires rest arguments to be used [enabled by default]
In file included from lsoda.c:72:0:
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:105:2: note: in expansion of macro ‘ERROR’
  ERROR(fmt, ## __VA_ARGS__); \
  ^
lsoda.c:569:6: note: in expansion of macro ‘hardfailure’
      hardfailure("[lsoda] itask = 4 or 5 and tcrit behind tout\n");
      ^
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:105:2: note: in expansion of macro ‘ERROR’
  ERROR(fmt, ## __VA_ARGS__); \
  ^
lsoda.c:596:6: note: in expansion of macro ‘hardfailure’
      hardfailure("[lsoda] ewt[%d] = %g <= 0.\n", i, _C(ewt)[i]);
      ^
lsoda.c:624:71: warning: ISO C99 requires rest arguments to be used [enabled by default]
      hardfailure("[lsoda] tout too close to t to start integration\n ");
                                                                       ^
lsoda.c:624:71: warning: ISO C99 requires rest arguments to be used [enabled by default]
In file included from lsoda.c:72:0:
lsoda_internal.h:18:53: warning: ISO C forbids conditional expr with only one void side [-Wpedantic]
 #define ERROR(fmt, ...) (ctx->error?free(ctx->error):1, ctx->error=_strdup_printf("EE:" fmt " @(%s:%d)", ## __VA_ARGS__, __FILE__, __LINE__))
                                                     ^
lsoda.c:105:2: note: in expansion of macro ‘ERROR’
  ERROR(fmt, ## __VA_ARGS__); \
  ^
lsoda.c:624:6: note: in expansion of macro ‘hardfailure’
      hardfailure("[lsoda] tout too close to t to start integration\n ");
      ^

[sdwfrost]

Dear @richfitz

Those macros were introduced by @rainwoodman; we could always revert these back to the original code, or just redefine them. Happy to drop GNU extensions etc. What would suit rlsoda etc.? Would you prefer a set of error codes defined by macros?

[rainwoodman]

It's been a few years. But I do recall the reason I added these ugly trinary operators was very likely because I wanted the initial reentrable version to be as close as the old code as possible.

I think it may as well be the right time to modernize the API -- spliting the error interface from the LSODA context, and probably rename the context class to LSODASolver. also, adding a LSODAError \ argument to each function call that raises an exception.

[sdwfrost]

Dear @rainwoodman

Thanks for the feedback. What would you suggest for the fields of a lsoda_error_t (I don't mind keeping the naming very old-c like)? Would it be worth using something like exceptions4c?

[richfitz]

What would suit rlsoda etc.? Would you prefer a set of error codes defined by macros?

My primary requirement is C99 compatibility. Ideally I'd like to avoid any calls to sprintf too as these will need to be patched (CRAN is super strict about these things, which is annoying for developers but does help with the platform independence).

In dde I use an enum of error codes (which would be an improvement of the magic numbers that trace back to the original Fortran -- similar also for itask which is pretty obscure atm).

[rainwoodman]

@sdwfrost I think that would be an overkill. I feel a split between the request and result would be good enough -- the exception can be returned in the result object -- something along these lines:

http://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.minimize.html#scipy.optimize.minimize

I haven't been actively using this package for many years, so it is really up to the boots on the ground to decide.

@richfitz Could you point to me some reference guide for coding style of CRAN? Is there an alternative to sprintf that is recommended? (printf would be even worse since it writes to stdout) We shall try to wrap these into the new implementation (assuming it will happen).

[richfitz]

Sorry - I mistyped; it's the use of printf and fprintf that is an issue (or anything that writes to stderr/stdout). The abort is also an issue but that's easily avoided.

The reference guide is "writing R extensions" which is long and rambling and at the same time not necessarily comprehensive, along with the CRAN repository policy. Practically it's just a case of running the code through R's QA tools and seeing where the grumbles are (there's a lot of culture clash within the old and new parts of the R community).

I agree with @rainwoodman that the exceptions4c looks like overkill. Also while I'm not a lawyer that could induce licence issues? (It's LGPL which I understand even less than GPL).

[sdwfrost]

So, should there be a separate error struct (which could contain arbitrary amounts of detail about the error) or should just a pointer to an int be passed in which to write an error code (removing the char* error in the context)?

[richfitz]

Given the varied nature of the messages, then a full struct seems the more natural choice

[rainwoodman]

Yes I agree with Rich. if we can do vsprintf then it is always a good idea to give more context. shall one of us propose a draft new API (just the header file) based on the current functionality, so that we have something to work on? The minimal requirement is R conformal and no loss of functionality so far we have discussed.

On Wed, Oct 12, 2016 at 1:46 AM, Rich FitzJohn notifications@github.com wrote:

Given the varied nature of the messages, then a full struct seems the more natural choice

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sdwfrost/liblsoda/issues/6#issuecomment-253155022, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIbTATngepYkdiRTz7LNnQbL4z40O2pks5qzJ54gaJpZM4KSwSU .

[sdwfrost]

Hi, I'm working on this over the next week. Any suggestions for an API to handle errors?

[rainwoodman]

doodling some ideas:

LSODASolver solver[1] = {{
      .function = function,
      .itask = 1, /* replace with better names + enum */
      .opt = 1,
    ....
},};
LSODAError error[1];

if(0 != lsoda_solver_init(solver, error)) {
     goto exc_init;
}

double tout = 1, t=0;
for(int i = 0; i < 10; i ++) {
    tout = tout * 10;

    if(0 != lsoda_solver_run(solver, &t, tout, error)) {
       goto exc_run;
    }
}
lsoda_solver_destroy(solver);
return 0;

exc_run:
         printf("%s\n", error->message);
         lsoda_error_destroy(error);
         lsoda_solver_destroy(solver);
return 1;
exc_init:
         printf("%s\n", error->message);
return 1;

[rainwoodman]

goto here serves as a 'try-catch' block.