[H-GEN] 'C' code help needed

Anthony Towns A.Towns at mailbox.uq.edu.au
Sat Oct 25 21:36:43 EDT 1997


On Sun, 26 Oct 1997, luke Grant wrote:

> Ok, i have this "c" assignment due in on tuesday and
> i am having trouble with dynamic allocation of memory.

Woohoo. Homework problems.

Still, I guess this is more relevant than legal queries, at any rate.

List nanny, are these on topic?

> I know that the question is not quite unix related but i figured 
> you guys would know what the problem was if anyone did.

If anyone does, the guys on comp.lang.c do.
 
> /*------------------------------------------------------------*/
> struct elements    /*global variable declaration*/
> {
>   char element[4];
>   int weight;
> };   
> static struct elements *elemdata;/*set struct pointer*/ 
> /*----------------------------------------------------*/ 
> 
> void memstruct(void)

:)

void f(void), the clearest statement ever devised to scream at you <<THIS
CHANGES GLOBAL DATA>>. I love it.

(Bring back fond memories of the ACM comp, guys?)

> {          
>  int index=0;
>  int size=2;
>  FILE *datafile;
>   datafile = fopen("atomwgh2.dat", "r"); 
>   if (datafile == (FILE *) NULL) 

The cast is unnecessay -- NULL is the right type for any sort of pointer,
and, more generally (in C, but not C++) void*'s can (and should) be
implicitly typecase rather than explicitly converted.

Calling NULL a void* isn't totally accurate, though.

Telling us the format of your data file would help also.

>   /*check file exists*/
>     {
>       printf("\n Failed to open the file named atomwght.dat. \n");
>       exit(1);
>     };
>  printf("size = %d\n",size);

2, right?
  
>  elemdata = (struct elements *) malloc(size * sizeof(struct elements));

Unless you want C++ compatability, drop the cast: it hides warnings that
might otherwise be useful.

You're not checking for the NULL pointer here. You should.

>  /*allocate memory for array of structure*/

(index == 0, here)
>  while (index > -1) 

Initialize index closer to where it's used.

>    {
>     if (fscanf(datafile,"%s",elemdata[index].element) == EOF)
>       {
>        elemdata[index].element[0]='\0';
>        index = -1;    /*set variable for exit of loop*/

_break_.

>        printf("reached end of file\n");
>        }
>       else   
>        {  
>         fscanf(datafile,"%d",elemdata[index].weight);
                               ^^^ needs an &
      
fgets followed by sscanf is, apparently, the recommended way of doing this
for robustness reasons.

>         printf("scanned in struct number %d\n",dummy);
                                                 ^^^^^ index?
>         size++;

It's probably better to make a good approximation at the size of the
buffer and make it that size to start with, then increase by larger
amounts when necessary -- realloc() is a costly operation.

(safestrcat() anyone?)

>         elemdata=(struct elements *) realloc(elemdata, size * sizeof(struct
> elements));

This isn't safe. If realloc() returns NULL the data at the original
location is still valid: realloc(3) on cello says...

   RETURN VALUES
        [...] When realloc() returns NULL, the block pointed to by ptr is
        left intact.   [...]

Which means you're left with a dangling pointer.

>                          /*add extra memory for string*/
                                                  ^^^^^^ struct? 
 
>         if (elemdata == (struct elements *) NULL) 
>            /*error message for no memory*/
>           {
>            printf("Failed to allocate extra memory!\n");
>            exit(1);
>           }
>         printf("successfuly alocated extra memory\n");
>         dummy++;
          ^^^^^ index? (as above)
>         maxnum++; /*increment variables for loop*/
          ^^^^^^ 'scuse me?
>        }
>      }
>  
>  fclose(datafile);
>  printf("I got this far\n");
>  
> }

I'd highly recommend _Writing Solid Code_ by Steve Maguire, published by 
Microsoft Press, which covers a number of risky C constructs slightly more
obscure than neglecting to NUL terminate your strings or putting "break"s
in your "case" statements.

Yes, I'd recommend it highly enough to use the word "Microsoft", on this
list, in a positive light, without even mixing it up with a Disney
trademark.

It's _that_ good.

Seriously.

_Code Complete_, by Steve (?) McConnell also published by MS Press isn't
bad either.

Cheers,
aj

----------------------- HUMBUG General List --------------------------------
echo "unsubscribe general" | mail majordomo at humbug.org.au # To Unsubscribe



More information about the General mailing list