473,692 Members | 1,959 Online
Bytes | Software Development & Data Engineering Community
+ Post

Home Posts Topics Members FAQ

Code Review? memory management in C

I have a file with different ways to write numbers

---- 8< (cut) --------
0: zero, zilch,, nada, ,,,, empty , void, oh
1: one
7: seven
2: two, too
---- >8 --------------

I wanted to read that file and put it into dynamic memory, like

char *** synonym;
/* assume everything is already malloc'd */
synonym[0][0] = "zero";
synonym[0][1] = "zilch";
/* ... */
synonym[4][0] = NULL;
/* ... */
synonym[7][0] = "seven";
synonym[7][1] = NULL;
synonym[8] = NULL;

After a few days (mostly nights) of struggling with it, the part of the
program that does that is done.

I welcome the approaching weekend to give it a rest before coding the
real reason for the program :)
Is it ok if I post 200 lines of newbie code, spread over three files,
for a code review on either alt.comp.lang.l earn.c-c++ or comp.lang.c?

--
If you're posting through Google read <http://cfaj.freeshell. org/google>
Feb 9 '06 #1
16 2316

"Pedro Graca" <he****@dodgeit .com> wrote in message
news:sl******** ***********@ID-203069.user.ind ividual.net...
I have a file with different ways to write numbers

---- 8< (cut) --------
0: zero, zilch,, nada, ,,,, empty , void, oh
1: one
7: seven
2: two, too
---- >8 --------------

I wanted to read that file and put it into dynamic memory, like

char *** synonym;
/* assume everything is already malloc'd */
synonym[0][0] = "zero";
synonym[0][1] = "zilch";
/* ... */
synonym[4][0] = NULL;
/* ... */
synonym[7][0] = "seven";
synonym[7][1] = NULL;
synonym[8] = NULL;

After a few days (mostly nights) of struggling with it, the part of the
program that does that is done.

I welcome the approaching weekend to give it a rest before coding the
real reason for the program :)
Is it ok if I post 200 lines of newbie code, spread over three files,
for a code review on either alt.comp.lang.l earn.c-c++ or comp.lang.c?


IMO that is a but much for a newsgroup post. I suggest
you put it on a web site, then post the URL here with
your comments/questions. Then those who are not interested
don't need to spend the time downloading, and those who are,
can.

-Mike
Feb 9 '06 #2
Mike Wahler wrote:
"Pedro Graca" <he****@dodgeit .com> wrote in message
news:sl******** ***********@ID-203069.user.ind ividual.net...
I have a file with different ways to write numbers <snip> I wanted to read that file and put it into dynamic memory <snip> Is it ok if I post 200 lines of newbie code, spread over three files,
for a code review on either alt.comp.lang.l earn.c-c++ or comp.lang.c?


IMO that is a but much for a newsgroup post. I suggest
you put it on a web site, then post the URL here with
your comments/questions. Then those who are not interested
don't need to spend the time downloading, and those who are,
can.


Ok. I posted the code to
<http://hexkid.blogspot .com/2006/02/memory-management-in-c.html>

My real question is if the program does not have memory leaks or some
other bad things with the allocation calls.

Other than that, any other hints/tips on how to do things better would
be very much appreciated.
PS. I had to tweak the code a bit (the '<'s and the '&'s) to have it
display properly. Hope it doesn't become an uncompilable mess for people
who copy/paste it to their computers.

--
If you're posting through Google read <http://cfaj.freeshell. org/google>
Feb 9 '06 #3
F'up2 clc

Pedro Graca wrote:
Ok. I posted the code to
<http://hexkid.blogspot .com/2006/02/memory-management-in-c.html>

My real question is if the program does not have memory leaks or some
other bad things with the allocation calls.

Other than that, any other hints/tips on how to do things better would
be very much appreciated.
I will comment on your main() function, so I quote that part here: /* main.c
*
* Pedro Graca, 2006-02-09
* This code is in the public domain
* */

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

int main(int argc, char * argv[])
char ** argv
is clearer.
{
char *** synonyms = NULL;
FILE * fp;
char line[MAX_LINE_LENGTH + 1];

if (argc < 2) {
fprintf(stderr, "Usage: %s FILENAME\n", argv[0]);
Note: argv[0] can be NULL. Make this
argv[0] ? argv[0] : "<program>"
or similar. exit(EXIT_FAILU RE);
}
Here, you have finished validating the input. Now, the real
thing starts. Put it into a function (with const char *
parameter) and call this function.
fp = fopen(argv[1], "r");
if (!fp) {
fprintf(stderr, "Unable to open file.\n");
exit(EXIT_FAILU RE);
}
while (fgets(line, MAX_LINE_LENGTH , fp)) {
if (parse_line(lin e, &synonyms))
fprintf(stderr, "Oops\n");
}
fclose(fp);
Note: If the file is completely empty, synonyms may still
be NULL here.
The same for errors in parse_line().

Obviously (by looking below, too), parse_line() does 2 things:
- If synonyms==NULL: Allocate synonyms (and maybe set
synonyms[0]=NULL); set some internal "line number" counter to 0.
- Always: replace synonyms[line number] by a pointer to the
parsed stuff, increase line number, set synonyms[line number]=NULL

This is one thing too much.
Break this down into
1) initialisation,
2) parsing, and
3) adding another item to the synonyms array.
{ /* validate program */
int nidx=0, syn_idx;

while (synonyms[nidx]) {
If synonyms is NULL, this will go terribly wrong.
syn_idx = 0;
printf("%d: ", nidx);
while (synonyms[nidx][syn_idx]) {
printf("%s", synonyms[nidx][syn_idx]);
++syn_idx;
if (synonyms[nidx][syn_idx])
printf(", ");
}
++nidx;
printf("\n");
}
}
Make this a function of its own.

{ /* free memory */
int nidx=0, syn_idx;

while (synonyms[nidx]) {
syn_idx = 0;
while (synonyms[nidx][syn_idx]) {
free(synonyms[nidx][syn_idx]);
++syn_idx;
}
free(synonyms[nidx][syn_idx]);
free(synonyms[nidx]);
++nidx;
}
free(synonyms[nidx]);
You forget to free(synonyms);
}
Rule of thumb: The role that allocates the stuff, frees the
stuff. Provide a dedicated allocation handling and an
accompanying deallocation function.
#ifdef _WIN32
printf("Press <ENTER>\n");
getchar();
#endif
Fair enough.
return 0;
}
All in all, rather nice.
Work at the design and break things down into easily testable
functions. Step 2: Have a test for these functions.
PS. I had to tweak the code a bit (the '<'s and the '&'s) to have it
display properly. Hope it doesn't become an uncompilable mess for people
who copy/paste it to their computers.


Provide a "nice to look at" as well as a download version.

Note: Consider using ggets()/fggets() for reading the lines.
These are non-standard but there is at least one public domain
version (by Chuck Falconer).
Advantage: No line length restriction, automatic allocation of
the memory for the line (which means no unnecessary copying in
your case).

I have looked only shortly at the actual selfref.c and
parse_line(). Do not try to save lines by putting if and
the statement following if (condition) in one line.
This easily can obscure things. Especially if one loses
indentation copying over your source from the browser.
The same holds for loops etc, too.
Cheers
Michael
--
E-Mail: Mine is an /at/ gmx /dot/ de address.
Feb 9 '06 #4
Pedro Graca wrote:

<snip>
Ok. I posted the code to
<http://hexkid.blogspot .com/2006/02/memory-management-in-c.html>

My real question is if the program does not have memory leaks or some
other bad things with the allocation calls.

Other than that, any other hints/tips on how to do things better would
be very much appreciated.
OK, I'll go through it and paste in the bits that I have comments on
together with the comments.
PS. I had to tweak the code a bit (the '<'s and the '&'s) to have it
display properly. Hope it doesn't become an uncompilable mess for people
who copy/paste it to their computers.


Yes, you do have to escape certain characters, but people get the
correct thing if they cut the text from the page and paste it to an
editor, so that is fine.

Not for code snippets and comments on them:

| if (argc < 2) {
| fprintf(stderr, "Usage: %s FILENAME\n", argv[0]);
| exit(EXIT_FAILU RE);
| }

argc could be 0 and argv[0] a null pointer, so you should check for and
handle that.

| char line[MAX_LINE_LENGTH + 1];
| ...
| while (fgets(line, MAX_LINE_LENGTH , fp)) {
| if (parse_line(lin e, &synonyms)) fprintf(stderr, "Oops\n");
| }

fgets expects the size of the buffer rather than the size of the buffer
- 1, so a simple:
while (fgets(line, sizeof line, fp)) {

You definitely have memory leaks. You have a couple of bits of code of
the form:
| words_list = malloc(sizeof *words_list); /* TODO: error checking */
| words = 0;
|
| ... (stuff that does not involve either word_list or words) ...
|
| while (buf[bufi]) {
|
| ... (stuff that does not involve either word_list or words) ...
|
| words_list[words] = word; /* words will still be 0 here */
| ++words;

So the first time through this while loop you overwrite the pointer
generated by the first malloc.

You need more error checking on your passing. I tried it on an invalid
file (one of the source files in fact!) and it gave no output at all.

As you note, you need error checking on the allocations.

I spotted the following:
| word = malloc(w_len + 1); /* TODO: error checking */
| strncpy(word, &(buf[w_start]), w_len);
| word[w_len] = 0;
There is no point using strncpy IMHO, you might as well use memcpy.

I've not checked the code thoroughly so there may well be other things.
--
Flash Gordon
Living in interesting times.
Although my email address says spam, it is real and I read it.
Feb 10 '06 #5
Michael Mair wrote:

F'up2 clc

Pedro Graca wrote:

int main(int argc, char * argv[])


char ** argv
is clearer.


int main(int argc, char *argv[])
.... is the form that I use,
because I copied it straight from the standard.
if (argc < 2) {
fprintf(stderr, "Usage: %s FILENAME\n", argv[0]);


Note: argv[0] can be NULL. Make this
argv[0] ? argv[0] : "<program>"
or similar.


I use puts instead of fprintf for that.

#define ARGV_0 "car"
#define OUTPUT ARGV_0 ".txt"

if (argc > 1) {

} else {
puts("Usage:\n >" ARGV_0 " <PART_FILE.tx t> ...\n\n"
}

--
pete
Feb 10 '06 #6
pete <pf*****@mindsp ring.com> writes:
Michael Mair wrote:
F'up2 clc

Pedro Graca wrote:

> int main(int argc, char * argv[])


char ** argv
is clearer.


int main(int argc, char *argv[])
... is the form that I use,
because I copied it straight from the standard.


Yes, the standard uses "char *argv[]", which is by definition
equivalent to "char **argv".

The [] form is intended to document the fact that the pointer argument
is a pointer to an array rather than to a single object.
Unfortunately, there's no enforcement of this -- I've never heard of a
compiler that even issues a warning based on this. (I'll note that
the language also supports comments, which can be used for just this
kind of thing.)

Personally, I prefer to use "char **argv" while keeping in mind C's
(admittedly overly complicated) rules about arrays and pointers. In
my opinion, allowing "char *argv[]" to be a synonym for "char **argv",
but only in the context of a parameter declaration, causes more
confusion than it cures. But that is, as I said, just my opinion, and
plenty of smart people unaccountably disagree with me. 8-)}

--
Keith Thompson (The_Other_Keit h) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.
Feb 10 '06 #7
Keith Thompson wrote:

pete <pf*****@mindsp ring.com> writes:
Michael Mair wrote:
F'up2 clc

Pedro Graca wrote:

> int main(int argc, char * argv[])

char ** argv
is clearer.


int main(int argc, char *argv[])
... is the form that I use,
because I copied it straight from the standard.


Yes, the standard uses "char *argv[]", which is by definition
equivalent to "char **argv".

The [] form is intended to document the fact that the pointer argument
is a pointer to an array rather than to a single object.
Unfortunately, there's no enforcement of this -- I've never heard of a
compiler that even issues a warning based on this. (I'll note that
the language also supports comments, which can be used for just this
kind of thing.)

Personally, I prefer to use "char **argv" while keeping in mind C's
(admittedly overly complicated) rules about arrays and pointers. In
my opinion, allowing "char *argv[]" to be a synonym for "char **argv",
but only in the context of a parameter declaration, causes more
confusion than it cures. But that is, as I said, just my opinion, and
plenty of smart people unaccountably disagree with me. 8-)}


The only reason that I write main
with an array looking parameter
is because it's shown that way in the standard.
I don't write any other functions that way.

I also use lower case str() and xstr() macros,
instead of upper case,
just because that's the way they are in the standard also.

--
pete
Feb 10 '06 #8
Michael Mair wrote:
.... snip ...
Note: Consider using ggets()/fggets() for reading the lines.
These are non-standard but there is at least one public domain
version (by Chuck Falconer).
Advantage: No line length restriction, automatic allocation of
the memory for the line (which means no unnecessary copying in
your case).


Available at <http://cbfalconer.home .att.net/download/ggets.zip>

--
"If you want to post a followup via groups.google.c om, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson
More details at: <http://cfaj.freeshell. org/google/>
Also see <http://www.safalra.com/special/googlegroupsrep ly/>
Feb 10 '06 #9
Pedro Graca wrote:
I have a file with different ways to write numbers

---- 8< (cut) --------
0: zero, zilch,, nada, ,,,, empty , void, oh
1: one
7: seven
2: two, too
---- >8 --------------
and later wrote:
Ok. I posted the code to
<http://hexkid.blogspot .com/2006/02/memory-management-in-c.html>

My real question is if the program does not have memory leaks or some
other bad things with the allocation calls.


Yes, I think there is a memory leak if you have two lines with the same
number:
5, five
5, cinco

The code does too many reallocs for my taste, but should function OK.

I appreciate your attempt to document the parameters for the functions.
I suggest you document the return values as well.

The pointer to data allocated for the first line will be overwritten by
the second.

--
Thad
Feb 10 '06 #10

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

2
1957
by: DANIEL BEAULIEU J | last post by:
Basically i am a student taking an operating systems course which is c++ intensive. Familiar with Java, and so not so familiar with memory management. Looking for suggestions of exercises or web resources to help me become familiar with pointers and referencing etc. Thank you.
24
5763
by: Robin Cole | last post by:
I'd like a code review if anyone has the time. The code implements a basic skip list library for generic use. I use the following header for debug macros: /* public.h - Public declarations and macros */ #ifndef PUBLIC #define PUBLIC
8
9327
by: Chad | last post by:
hello, i am losing memory each time i make this call to a C++ dll (I make frequent calls). 'in VB.Net Declare Function grab Lib "grabber.dll" _ (ByRef lpBuf As Byte, ByVal lnum As Integer) As Integer the C++ function is...
1
2796
by: trialproduct2004 | last post by:
Hi all, I am having slight confusion regarding memory management in .net. Say suppose i have two application one is in C# and other is in MFC(VC++). Both of this application are using lots of memory. Suppose i run first C# application which has occupied all memory and
0
1932
by: erez_acount | last post by:
***************************************************************************** Call For Papers The 2006 International Symposium on Memory Management (ISMM'06) Co-located with PLDI 2006 Ottawa, Canada June 10-11 2006
94
4724
by: smnoff | last post by:
I have searched the internet for malloc and dynamic malloc; however, I still don't know or readily see what is general way to allocate memory to char * variable that I want to assign the substring that I found inside of a string. Any ideas?
263
9248
by: Malcolm McLean | last post by:
The webpages for my new book are now up and running. The book, Basic Algorithms, describes many of the fundamental algorithms used in practical programming, with a bias towards graphics. It includes mathematical routines from the basics up, including floating point arithmetic, compression techniques, including the GIF and JPEG file formats, hashing, red black trees, 3D and 3D graphics, colour spaces, machine learning with neural...
27
2946
by: George2 | last post by:
Hello everyone, Should I delete memory pointed by pointer a if there is bad_alloc when allocating memory in memory pointed by pointer b? I am not sure whether there will be memory leak if I do not delete a. try { a = new int ;
0
1646
by: corey | last post by:
Secure Bytes audit and vulnerability assessment software Secure Auditor named “Versatile tool” and earn “Five Star Ratings” in SC Magazine Group Test Secure Bytes is really pleased to share this great news with its associates that Secure Auditor has been branded as a Five Star product by SC Magazine August 2008 edition. SC Magazine is among the world’s most prestigious Information Security magazines. In the comparative review with...
0
8535
by: Hystou | last post by:
Most computers default to English, but sometimes we require a different language, especially when relocating. Forgot to request a specific language before your computer shipped? No problem! You can effortlessly switch the default language on Windows 10 without reinstalling. I'll walk you through it. First, let's disable language synchronization. With a Microsoft account, language settings sync across devices. To prevent any complications,...
0
8796
tracyyun
by: tracyyun | last post by:
Dear forum friends, With the development of smart home technology, a variety of wireless communication protocols have appeared on the market, such as Zigbee, Z-Wave, Wi-Fi, Bluetooth, etc. Each protocol has its own unique characteristics and advantages, but as a user who is planning to build a smart home system, I am a bit confused by the choice of these technologies. I'm particularly interested in Zigbee because I've heard it does some...
0
7626
agi2029
by: agi2029 | last post by:
Let's talk about the concept of autonomous AI software engineers and no-code agents. These AIs are designed to manage the entire lifecycle of a software development project—planning, coding, testing, and deployment—without human intervention. Imagine an AI that can take a project description, break it down, write the code, debug it, and then launch it, all on its own.... Now, this would greatly impact the work of software developers. The idea...
1
6458
isladogs
by: isladogs | last post by:
The next Access Europe User Group meeting will be on Wednesday 1 May 2024 starting at 18:00 UK time (6PM UTC+1) and finishing by 19:30 (7.30PM). In this session, we are pleased to welcome a new presenter, Adolph Dupré who will be discussing some powerful techniques for using class modules. He will explain when you may want to use classes instead of User Defined Types (UDT). For example, to manage the data in unbound forms. Adolph will...
0
5817
by: conductexam | last post by:
I have .net C# application in which I am extracting data from word file and save it in database particularly. To store word all data as it is I am converting the whole word file firstly in HTML and then checking html paragraph one by one. At the time of converting from word file to html my equations which are in the word document file was convert into image. Globals.ThisAddIn.Application.ActiveDocument.Select();...
0
4322
by: TSSRALBI | last post by:
Hello I'm a network technician in training and I need your help. I am currently learning how to create and manage the different types of VPNs and I have a question about LAN-to-LAN VPNs. The last exercise I practiced was to create a LAN-to-LAN VPN between two Pfsense firewalls, by using IPSEC protocols. I succeeded, with both firewalls in the same network. But I'm wondering if it's possible to do the same thing, with 2 Pfsense firewalls...
0
4555
by: adsilva | last post by:
A Windows Forms form does not have the event Unload, like VB6. What one acts like?
2
2234
muto222
by: muto222 | last post by:
How can i add a mobile payment intergratation into php mysql website.
3
1955
bsmnconsultancy
by: bsmnconsultancy | last post by:
In today's digital era, a well-designed website is crucial for businesses looking to succeed. Whether you're a small business owner or a large corporation in Toronto, having a strong online presence can significantly impact your brand's success. BSMN Consultancy, a leader in Website Development in Toronto offers valuable insights into creating effective websites that not only look great but also perform exceptionally well. In this comprehensive...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.