Re: Dangerous assert() usage, parsed value count and stdbool.h compatibility
3 nov 2010 kl. 16.56 skrev Jens Rehsack:
> Hi Martin, hi Nathan,
> because confuse-devel@ still denies mails from non-members, I continue
> to cc you.
> I currently stepped into several "issues" in libConfuse - but before
> spending effort on
> fixing them, I'd like to read the maintainers/authors opinion.
> 1) I've seen during some code checking, that only assert() is used to prove for
> NULL-pointers. But assert is expanded to a no-op when NDEBUG is defined,
> which might be default for some production compiles.
> Is it intended that in such circumstances (e.g. typo in opt-name when calling
> cfg_set_validate_func) libconfuse simply segfaults?
If -DNDEBUG is used (why would anyone want to disable error checking!?), I guess it's deliberate. IMHO, the behaviour of having -DNDEBUG disable assert was a mistake.
> I suggest a macro 'assure(check, error-value)' which returns
> error-value if the
> test fails (in case of -DNDEBUG). Probably abort() after displaying an error
> could be suitable, too (even I this is definitively not a preferred way ...).
My intention with aborting was to make it easier to spot errors in calling the library.
> 2) struct cfg_opt_t.nvalues is documented as the number of parsed values, but
> it's used as the amount of values in struct cfg_opt_t.values.
> I miss a call to detect whether an option was included in the
> configuration file
> or not (e.g. to differ if one wants to suppress logging by set
> log-file = "" or if
> simply no entry of log-file was included in the parsed config).
> As workaround I use currently a null pointer as default value for
> strings, but
> this doesn't work for sections.
> My suggestions would be to have a separate counter for parsed values in
> struct cfg_opt_t.
> 3) libConfuse has an incompatible boolean type compared to stdbool.h.
> I'd like to see there some defines to the stdbool.h boolean types and values,
> when available - and the libConfuse own own ones only on compilers
> before C99.
IIRC, recent commits made libconfuse dependent on C99. So either a simple int or the bool from stdbool.h seems reasonable.
> I can spent some effort to fix above issues if you want.