Re: Error handling library

Andrew Poelstra wrote:
I hammered this out this morning to fix inconsistancies with the way my
programs handle errors. The code itself is fine, in that it compiles with
Richard Heathfield's gcc tags (plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

Aother poster here brought this up; I'd suggest
just prefix H_ ont your header names for this; its much
easier than memorising the rules for #define.


#include <stdlib.h> /* Needed for size_t */

/* Diagnostic severity */
#define ADL_INFO 0
#define ADL_WARN 1
#define ADL_BENIGN 2
#define ADL_FATAL 3


1. I don't really know why this is #defined; for a type
that needs to hold a limited range of values, C provides
enum (although I suppose that its a matter of taste?)
which lets the compiler catch out-of-range usage.

For example, if the above was an enum:
the compiler will prevent a caller from passing 5 to
register_error for the argument fatal/sev (see point #2).
Since you never check that "severity" is in range before
you use it as an index into an array, you run the very real
risk of a programmer checking the prototype of the function
and assuming that a higher int means "more dangerous error"
(true up to a point with your values) and merely upping the
int value of the argument without realising that there is
a limit (or even #defined values).

struct error_t;
struct error_l;

int adl_set_buffer (size_t size);
int adl_register_error (char *file, char *error, int line, int fatal);

2. The prototype for register_error differs in the name of
the last argument; I doubt that any harm will come from this
but you make the maintainers life harder (eg. automated methods
for finding a prototype from a function definition or vice
versa may not work as they differ).

void adl_terminate (int display);

struct error_t
{
char *text;
char *file;
int line;
int severity;
};

struct error_l
{
struct error_t *err;
size_t ctr;
size_t max;
int wrap;
};

const char *adl_message[] = {"Info", "Warning", "Error", "Fatal error"};

3. I'd change this (together with defining an enum above) so that
the lookup table and the range are are textually close together
with a comment warning that one is used as an index into the array
(which is the other one). I'll put an example down below at the end
of this post. On a related note, do you think that defining an array
in a header is a good idea? All the modules which include this header
will have their own copy of the array but only this module will
ever use its copy of the array.


struct error_l *adl_err = NULL;

#endif
/* End of header */

/* Start of source error.c */
#include "error.h"

3.5 (bit of a nitpick only) It's generally accepted to
put standard headers first, then system headers and then
local (to the project) headers.

#include <stdio.h>
#include <stdlib.h>


int adl_set_buffer (size_t size)
{
adl_err = malloc (sizeof *adl_err);
if (adl_err == NULL)
return -1;

adl_err->err = malloc (sizeof *adl_err->err * size);
adl_err->max = size;
adl_err->ctr = 0;

if (adl_err->err == NULL)
{
free (adl_err);
adl_err = NULL;
return -1;
}

return 0;
}

4. Seeing as how you are only returning one of two values
in this function, you might make that clear to the maintainer
by making return type of this function a bool. You would
lose nothing but would gain some clarity for the reader.



int adl_register_error (char *file, char *error, int line, int sev)
{
unsigned i = adl_err->ctr; /* This is here for readability only. */

if (adl_err == NULL)
return -1;

adl_err->err[i].text = error;
adl_err->err[i].file = file;
adl_err->err[i].line = line;
adl_err->err[i].severity = sev;

i++;

if (i > adl_err->max)
{
adl_err->ctr = 0;
adl_err->wrap = 1;
}
else
adl_err->ctr = i;

return 0;
}


5. See point #4 above.


void adl_terminate (int display)

6. This is not properly named (much like your other functions
as was pointed out by other posters) because it won't
always terminate, see? Why not make two different functions,
one that does termination and one that does display?

{
unsigned ctr;
struct error_t *cur; /* Replaces adl_err->err[ctr] for readability. */

/* If we aren't writing anything to the screen, simply terminate. */
if (adl_err == NULL || !display)
exit (EXIT_FAILURE);

/* If we've wrapped, start immediately past last error. */
ctr = adl_err->wrap ? (adl_err->ctr + 1) : 0;

/* Loop ends when we reach last error */
while (ctr != adl_err->ctr)
{
cur = &adl_err->err[ctr];
printf ("%s: %s (line %d in %s).\n",
adl_message[cur->severity],
cur->text, cur->line, cur->file);

ctr++;
if (ctr == adl_err->max) /* We need this if we're wrapping*/
ctr = 0;
}
}
/* End of source */


Right. Seeing that a lookup table for user messages is something
that you will likely need in most non-trivial projects, you can
try something like this (I'm using your library as an example):


Warning: I've typeed this straight into my web-browser,
it has not been compiled or tested in any way.

/* ------------- start Header file ------------- */
#ifndef H_ERROR
#define H_ERROR

enum error_type_t {
ADL_INFO = 0,
ADL_WARN,
ADL_BENIGN,
ADL_FATAL,
/* New error types can be added here only
* if the corresponding entry in adl_reror_lookup()
* is modified as well. For every entry here, there
* must be one in adl_error_lookup() as well.
*/
ADL_RANGE
};

char *adl_error_lookup (error_type_t err);

#endif
/* ------------- end Header file ------------- */

/* ------------- start Source file ------------- */
/* First include all your standard headers here */

/* Include the system headers here */

#include "error.h"

char *adl_error_lookup (error_type_t err)
{
static char *message[] = {
"Info",
"Warning",
"Error",
"Fatal error",
/* New entries must be added in here. For every entry
* in here there must a corresponding entry in the
* definition of error_type_t.
*/
"Unknown",
};

return message[err];
}

/* ------------- end Source file ------------- */

See, the advantage here is we don't even need to check that
the argument in adl_error_lookup() is within the range of
the messages in the array, the compiler will do that auto-
matically for us. Also note the comment I've placed at the
two points so that the maintainer *knows* that he must
modify something else when he adds new messages or new error
types.

You can modify this to easily fit something else; I'm
using this to store *all* messages directed at the user
for an embedded application so that I can easily swap
languages while running (an array of languages, each
consisting of the above array of messages; array of
languages is indexed with a variable "curr_lang" which is
of an enum type with range of only the languages supported).

With care (and a filesystem on your platform), you can
have the above method change languages with the languages
stored on your filesystem instead of in memory.


HTH
goose

.

Relevant Pages

  • Re: Maximum char array size?
    ... versus declaring an array at compile time (other than the need ... I did not get anywhere before I declared the new type ARRAY2. ... and DMC (Digital Mars Compiler) on WinXp. ... void PrintArray(char *name, int *array, int ncol, int nrow) ...
    (comp.lang.c)
  • Re: Fridays the thirteenth. (And a little puzzle.)
    ... -- compiler) is the usual method ... int febdays ... -- We're going to go round a loop dealing with each year in turn. ... -- other languages call) ...
    (uk.people.silversurfers)
  • Re: On VLAs and incomplete types
    ... I don't understand how a variable is handled by the compiler in ... declaring a VLA. ... an array must be declared with an ... int x; ...
    (comp.lang.c)
  • Re: max size of ptr array
    ... you'll get a compiler error. ... number you are allowed type in the brackets is exactly 536870911. ... int main{ ... And since an int requires 4 "bytes" plus the fact that your array is ...
    (microsoft.public.vc.language)
  • random number code
    ... Will it really compile exactly the same as mingw compiler used ... // initialization of static private members ... MSBs of the seed affect only MSBs of the array ... // constructor with 32 bit int as seed ...
    (comp.lang.cpp)