Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Request for source code review

Reply
Thread Tools

Request for source code review

 
 
Jan Kuiken
Guest
Posts: n/a
 
      08-31-2012
On 8/29/12 21:19 , Udyant Wig wrote:

....

> Being a novice, I request the readers of this newsgroup to look over the
> source to suggest improvements regarding readability, layout, style,
> correctness, idioms, etc. I would appreciate it very much.


Very readable code, good code comments.

One point though, AFAIK the keywords 'extern' in the header files are
not necessary.

Jan Kuiken


 
Reply With Quote
 
 
 
 
Keith Thompson
Guest
Posts: n/a
 
      08-31-2012
Udyant Wig <> writes:
[...]
> #define NDICE 5 /* The number of dice to be rolled. */
> #define SIDES 6 /* Each die has these many sides. */
> #define MAXSUM (SIDES * NDICE) /* The maximum possible sum of NDICE
> dice.
> */
> #define LIMIT ((MAXSUM) + 1) /* This constant is used:
> 0) to declare the array of sums
> 1) as a loop limit
> */

[...]

The inner parentheses in the definition of LIMIT are unnecessary.
As long as MAXSUM is defined properly (which it is), you can
just write:

#define LIMIT (MAXSUM + 1)

In general, you don't need to parenthesize single tokens
(identifiers, constants) in macro definitions.

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
Will write code for food.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
 
 
 
Les Cargill
Guest
Posts: n/a
 
      09-01-2012
Ian Collins wrote:
> On 08/30/12 11:43 PM, James Kuyper wrote:
>> On 08/30/2012 01:04 AM, Ian Collins wrote:
>>> On 08/30/12 01:55 PM, Keith Thompson wrote:

>> ....
>>>> This isn't directly a C issue, but your Makefile doesn't reflect any
>>>> dependencies on the *.h files. For example, if you edit gauss.h and
>>>> type "make", it won't rebuild anything.
>>>
>>> A decent make should provide a means for automating this. Otherwise
>>> where do you stop recursing up the include paths?

>>
>> I've used a "decent" make that was part of a configuration management
>> system called ClearCase, which fully automated the tracking of
>> dependencies, but by rather drastic means. Whenever clearmake executed
>> the build script for a given target, it would monitor the processes that
>> were executed, keeping track of ALL files that were opened during the
>> build. If they were versioned files managed by ClearCase, it identified
>> which version of the file was currently visible; for other files it just
>> looked at the file date. The next time that the same target was built,
>> it would check to see whether any of the files that had been opened the
>> last time had been changed, and if so, it assumed that the build script
>> had to be re-executed.
>>
>> One of the key things that made this work was that the make facility was
>> tightly integrated with a special device driver that managed access to
>> disk space; only the use of files stored on disk volumes managed by that
>> driver could be detected.

>
> I've had hours of fun with clearmake...
>
> Sun's (d)make uses the output of the preprocessor phase of compiles to
> collate dependency information. Rather more elegant and a lot cheaper!
>


*nix "make" makefiles can easily say :
depend:
$(CC) -M $(CFLAGS) *.c > .depend
....

include .depend


--
Les Cargill
 
Reply With Quote
 
Adrian Ratnapala
Guest
Posts: n/a
 
      09-01-2012
On 2012-08-30, Ian Collins <ian-> wrote:

>> This isn't directly a C issue, but your Makefile doesn't reflect any
>> dependencies on the *.h files. For example, if you edit gauss.h and
>> type "make", it won't rebuild anything.

>
> A decent make should provide a means for automating this. Otherwise
> where do you stop recursing up the include paths?


And when I use an *indecent* make, I actually don't worry about this.
Whenver I do an "important" build ("make dist", or whatever) then
automated tools do a complete rebuild.

For the tight edit-test-fix cycle, I just try to keep the dependencies
in my head, and do a "make clean" whenever I see a problem. Of course
I often get it wrong, but I do a "make clean" often enough that it
doesn't get out of hand.

 
Reply With Quote
 
Adrian Ratnapala
Guest
Posts: n/a
 
      09-01-2012
> The inner parentheses in the definition of LIMIT are unnecessary.
> As long as MAXSUM is defined properly (which it is), you can
> just write:
>
> #define LIMIT (MAXSUM + 1)
>
> In general, you don't need to parenthesize single tokens
> (identifiers, constants) in macro definitions.


I suspect the reason the original poster put MAXSUM in parentheses is
because he had read that macro *arguments* need to be a protected.
And that is correct!

You *do* need to parethesize arguments. Thus you need to do

#define MULTIPLY_THE_HARD_WAY(A, B) ( (A) * (B) )

because otherwise MULTIPLY_THE_HARD_WAY(x+y, 4) would expand to (x +
y*4); which would be wrong.


 
Reply With Quote
 
Keith Thompson
Guest
Posts: n/a
 
      09-01-2012
Adrian Ratnapala <> writes:
>> The inner parentheses in the definition of LIMIT are unnecessary.
>> As long as MAXSUM is defined properly (which it is), you can
>> just write:
>>
>> #define LIMIT (MAXSUM + 1)
>>
>> In general, you don't need to parenthesize single tokens
>> (identifiers, constants) in macro definitions.

>
> I suspect the reason the original poster put MAXSUM in parentheses is
> because he had read that macro *arguments* need to be a protected.
> And that is correct!
>
> You *do* need to parethesize arguments. Thus you need to do
>
> #define MULTIPLY_THE_HARD_WAY(A, B) ( (A) * (B) )
>
> because otherwise MULTIPLY_THE_HARD_WAY(x+y, 4) would expand to (x +
> y*4); which would be wrong.


You're right, of course. Single tokens in macro definitions don't
generally need to be parenthesized *unless* they're macro arguments.
Silly of me to miss that.

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
Will write code for food.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
James Kuyper
Guest
Posts: n/a
 
      09-02-2012
On 08/31/2012 10:18 PM, Les Cargill wrote:
> Ian Collins wrote:
>> On 08/30/12 11:43 PM, James Kuyper wrote:
>>> On 08/30/2012 01:04 AM, Ian Collins wrote:
>>>> On 08/30/12 01:55 PM, Keith Thompson wrote:
>>> ....
>>>>> This isn't directly a C issue, but your Makefile doesn't reflect any
>>>>> dependencies on the *.h files. For example, if you edit gauss.h and
>>>>> type "make", it won't rebuild anything.
>>>>
>>>> A decent make should provide a means for automating this. Otherwise
>>>> where do you stop recursing up the include paths?
>>>
>>> I've used a "decent" make that was part of a configuration management
>>> system called ClearCase, which fully automated the tracking of
>>> dependencies, but by rather drastic means. Whenever clearmake executed
>>> the build script for a given target, it would monitor the processes that
>>> were executed, keeping track of ALL files that were opened during the
>>> build. If they were versioned files managed by ClearCase, it identified
>>> which version of the file was currently visible; for other files it just
>>> looked at the file date. The next time that the same target was built,
>>> it would check to see whether any of the files that had been opened the
>>> last time had been changed, and if so, it assumed that the build script
>>> had to be re-executed.
>>>
>>> One of the key things that made this work was that the make facility was
>>> tightly integrated with a special device driver that managed access to
>>> disk space; only the use of files stored on disk volumes managed by that
>>> driver could be detected.

>>
>> I've had hours of fun with clearmake...
>>
>> Sun's (d)make uses the output of the preprocessor phase of compiles to
>> collate dependency information. Rather more elegant and a lot cheaper!
>>

>
> *nix "make" makefiles can easily say :
> depend:
> $(CC) -M $(CFLAGS) *.c > .depend
> ...
>
> include .depend


I tried that for several years. It ended up being more trouble than it
was worth, partly because .depend was always one version out of date
compared to the current code. Unfortunately, I don't remember the details.

What clearmake did was much more comprehensive - it tracked down all
kinds of file dependencies, not just header files, (which can be helpful
when the build script included commands other than those that invoke a
compiler) and the user never had to deal with how or where that
information was stored. As I remember, it "just worked" (at least, the
dependency tracking did - other parts of the system were a little more
complicated to deal with).
--
James Kuyper
 
Reply With Quote
 
Les Cargill
Guest
Posts: n/a
 
      09-02-2012
James Kuyper wrote:
> On 08/31/2012 10:18 PM, Les Cargill wrote:
>> Ian Collins wrote:
>>> On 08/30/12 11:43 PM, James Kuyper wrote:
>>>> On 08/30/2012 01:04 AM, Ian Collins wrote:
>>>>> On 08/30/12 01:55 PM, Keith Thompson wrote:
>>>> ....
>>>>>> This isn't directly a C issue, but your Makefile doesn't reflect any
>>>>>> dependencies on the *.h files. For example, if you edit gauss.h and
>>>>>> type "make", it won't rebuild anything.
>>>>>
>>>>> A decent make should provide a means for automating this. Otherwise
>>>>> where do you stop recursing up the include paths?
>>>>
>>>> I've used a "decent" make that was part of a configuration management
>>>> system called ClearCase, which fully automated the tracking of
>>>> dependencies, but by rather drastic means. Whenever clearmake executed
>>>> the build script for a given target, it would monitor the processes that
>>>> were executed, keeping track of ALL files that were opened during the
>>>> build. If they were versioned files managed by ClearCase, it identified
>>>> which version of the file was currently visible; for other files it just
>>>> looked at the file date. The next time that the same target was built,
>>>> it would check to see whether any of the files that had been opened the
>>>> last time had been changed, and if so, it assumed that the build script
>>>> had to be re-executed.
>>>>
>>>> One of the key things that made this work was that the make facility was
>>>> tightly integrated with a special device driver that managed access to
>>>> disk space; only the use of files stored on disk volumes managed by that
>>>> driver could be detected.
>>>
>>> I've had hours of fun with clearmake...
>>>
>>> Sun's (d)make uses the output of the preprocessor phase of compiles to
>>> collate dependency information. Rather more elegant and a lot cheaper!
>>>

>>
>> *nix "make" makefiles can easily say :
>> depend:
>> $(CC) -M $(CFLAGS) *.c > .depend
>> ...
>>
>> include .depend

>
> I tried that for several years. It ended up being more trouble than it
> was worth, partly because .depend was always one version out of date
> compared to the current code. Unfortunately, I don't remember the details.



You only need to "make depend" when a file has its includes changed. And
in a pinch, you can always utter "gcc -M sourcefile.c >> .depend".

>
> What clearmake did was much more comprehensive - it tracked down all
> kinds of file dependencies, not just header files, (which can be helpful
> when the build script included commands other than those that invoke a
> compiler) and the user never had to deal with how or where that
> information was stored.


I would have to use it for months before I could trust it.

> As I remember, it "just worked" (at least, the
> dependency tracking did - other parts of the system were a little more
> complicated to deal with).
>


Never used clearmake; only clearcase. Was less than impressed with
clearcase.

--
Les Cargill

 
Reply With Quote
 
Ian Collins
Guest
Posts: n/a
 
      09-02-2012
On 09/ 2/12 01:45 PM, Les Cargill wrote:
> James Kuyper wrote:
>> On 08/31/2012 10:18 PM, Les Cargill wrote:
>>> Ian Collins wrote:
>>>> On 08/30/12 11:43 PM, James Kuyper wrote:
>>>>> On 08/30/2012 01:04 AM, Ian Collins wrote:
>>>>>> On 08/30/12 01:55 PM, Keith Thompson wrote:
>>>>> ....
>>>>>>> This isn't directly a C issue, but your Makefile doesn't reflect any
>>>>>>> dependencies on the *.h files. For example, if you edit gauss.h and
>>>>>>> type "make", it won't rebuild anything.
>>>>>>
>>>>>> A decent make should provide a means for automating this. Otherwise
>>>>>> where do you stop recursing up the include paths?
>>>>>
>>>>> I've used a "decent" make that was part of a configuration management
>>>>> system called ClearCase, which fully automated the tracking of
>>>>> dependencies, but by rather drastic means. Whenever clearmake executed
>>>>> the build script for a given target, it would monitor the processes that
>>>>> were executed, keeping track of ALL files that were opened during the
>>>>> build. If they were versioned files managed by ClearCase, it identified
>>>>> which version of the file was currently visible; for other files it just
>>>>> looked at the file date. The next time that the same target was built,
>>>>> it would check to see whether any of the files that had been opened the
>>>>> last time had been changed, and if so, it assumed that the build script
>>>>> had to be re-executed.
>>>>>
>>>>> One of the key things that made this work was that the make facility was
>>>>> tightly integrated with a special device driver that managed access to
>>>>> disk space; only the use of files stored on disk volumes managed by that
>>>>> driver could be detected.
>>>>
>>>> I've had hours of fun with clearmake...
>>>>
>>>> Sun's (d)make uses the output of the preprocessor phase of compiles to
>>>> collate dependency information. Rather more elegant and a lot cheaper!
>>>>
>>>
>>> *nix "make" makefiles can easily say :
>>> depend:
>>> $(CC) -M $(CFLAGS) *.c> .depend
>>> ...
>>>
>>> include .depend

>>
>> I tried that for several years. It ended up being more trouble than it
>> was worth, partly because .depend was always one version out of date
>> compared to the current code. Unfortunately, I don't remember the details.

>
>
> You only need to "make depend" when a file has its includes changed. And
> in a pinch, you can always utter "gcc -M sourcefile.c>> .depend".


Sticking one instance of .KEEP_STATE: in your makefile is way more elegant.

--
Ian Collins
 
Reply With Quote
 
Les Cargill
Guest
Posts: n/a
 
      09-02-2012
Ian Collins wrote:
> On 09/ 2/12 01:45 PM, Les Cargill wrote:
>> James Kuyper wrote:
>>> On 08/31/2012 10:18 PM, Les Cargill wrote:
>>>> Ian Collins wrote:
>>>>> On 08/30/12 11:43 PM, James Kuyper wrote:
>>>>>> On 08/30/2012 01:04 AM, Ian Collins wrote:
>>>>>>> On 08/30/12 01:55 PM, Keith Thompson wrote:
>>>>>> ....
>>>>>>>> This isn't directly a C issue, but your Makefile doesn't reflect
>>>>>>>> any
>>>>>>>> dependencies on the *.h files. For example, if you edit gauss.h
>>>>>>>> and
>>>>>>>> type "make", it won't rebuild anything.
>>>>>>>
>>>>>>> A decent make should provide a means for automating this. Otherwise
>>>>>>> where do you stop recursing up the include paths?
>>>>>>
>>>>>> I've used a "decent" make that was part of a configuration management
>>>>>> system called ClearCase, which fully automated the tracking of
>>>>>> dependencies, but by rather drastic means. Whenever clearmake
>>>>>> executed
>>>>>> the build script for a given target, it would monitor the
>>>>>> processes that
>>>>>> were executed, keeping track of ALL files that were opened during the
>>>>>> build. If they were versioned files managed by ClearCase, it
>>>>>> identified
>>>>>> which version of the file was currently visible; for other files
>>>>>> it just
>>>>>> looked at the file date. The next time that the same target was
>>>>>> built,
>>>>>> it would check to see whether any of the files that had been
>>>>>> opened the
>>>>>> last time had been changed, and if so, it assumed that the build
>>>>>> script
>>>>>> had to be re-executed.
>>>>>>
>>>>>> One of the key things that made this work was that the make
>>>>>> facility was
>>>>>> tightly integrated with a special device driver that managed
>>>>>> access to
>>>>>> disk space; only the use of files stored on disk volumes managed
>>>>>> by that
>>>>>> driver could be detected.
>>>>>
>>>>> I've had hours of fun with clearmake...
>>>>>
>>>>> Sun's (d)make uses the output of the preprocessor phase of compiles to
>>>>> collate dependency information. Rather more elegant and a lot
>>>>> cheaper!
>>>>>
>>>>
>>>> *nix "make" makefiles can easily say :
>>>> depend:
>>>> $(CC) -M $(CFLAGS) *.c> .depend
>>>> ...
>>>>
>>>> include .depend
>>>
>>> I tried that for several years. It ended up being more trouble than it
>>> was worth, partly because .depend was always one version out of date
>>> compared to the current code. Unfortunately, I don't remember the
>>> details.

>>
>>
>> You only need to "make depend" when a file has its includes changed. And
>> in a pinch, you can always utter "gcc -M sourcefile.c>> .depend".

>
> Sticking one instance of .KEEP_STATE: in your makefile is way more elegant.
>


Neat! I'd agree.

That appears to be specific to Sun make.

--
Les Cargill

 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
What is code review? (Java code review) www Java 51 05-15-2007 01:10 PM
Code Review Request pitoniakm@msn.com Java 2 06-30-2005 02:26 PM
Request for Code Review Ben Hanson C++ 19 07-03-2004 11:27 PM
Request for code review P Kenter C++ 4 06-02-2004 05:26 PM
Re: Accessing Request.InputStream / Request.BinaryRead *as the request is occuring*: How??? Brian Birtle ASP .Net 2 10-16-2003 02:11 PM



Advertisments
 



1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57