Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Bug/Gross InEfficiency in HeathField's fgetline program

Reply
Thread Tools

Bug/Gross InEfficiency in HeathField's fgetline program

 
 
Richard
Guest
Posts: n/a
 
      10-08-2007
Richard Heathfield <(E-Mail Removed)> writes:

> Antoninus Twink said:
>
>> The function below is from Richard HeathField's fgetline program.

>
> It appears to be from my emgen utility, in fact.
>
>> For
>> some reason, it makes three passes through the string (a strlen(), a
>> strcpy() then another pass to change dots) when two would clearly be
>> sufficient. This could lead to unnecessarily bad performance on very
>> long strings.

>
> If it becomes a problem, I'll fix it. So far, it has not been a
> problem.


Why write it to be slower for the same functionality. It's basic level 1
pointer stuff.

>
>> It is also written in a hard-to-read and clunky style.

>
> A matter of opinion. Which bit did you find hard to read?


It covers about 20 lines - this is harder to read when the other is
about 4 and easy to follow. In my experience excessive white space is rarely welcome in
the real world especially when using a debugger.

>
>> char *dot_to_underscore(const char *s)
>> {
>> char *t = malloc(strlen(s) + 1);
>> if(t != NULL)
>> {
>> char *u;
>> strcpy(t, s);
>> u = t;
>> while(*u)
>> {
>> if(*u == '.')
>> {
>> *u = '_';
>> }
>> ++u;
>> }
>> }
>> return
>> t;
>> }
>>
>> Proposed solution:
>>
>> char *dot_to_underscore(const char *s)
>> {
>> char *t, *u;
>> if(t=u=malloc(strlen(s)+1))
>> while(*u++=(*s=='.' ? s++, '_' : *s++));
>> return t;
>> }

>
> It is not obvious to me that this code correctly replaces the code I
> wrote.


Which part do you think doesn't "correctly" replace the code? Always
better to highlight the errors since we are all prone to making them. I
guess there is some feature in your code that you are alluding too that
is not obvious to others.
 
Reply With Quote
 
 
 
 
Richard
Guest
Posts: n/a
 
      10-08-2007
Thad Smith <(E-Mail Removed)> writes:

>>>>
>>>> char *dot_to_underscore(const char *s)
>>>> {
>>>> char *t, *u;
>>>> if(t=u=malloc(strlen(s)+1))
>>>> while(*u++=(*s=='.' ? s++, '_' : *s++));
>>>> return t;
>>>> }

>
> What happens when malloc returns a null pointer?


It returns null obviously , why?
 
Reply With Quote
 
 
 
 
Antoninus Twink
Guest
Posts: n/a
 
      10-08-2007
On 8 Oct 2007 at 9:04, Philip Potter wrote:
> Antoninus Twink wrote:
>> On 7 Oct 2007 at 22:55, Richard Heathfield wrote:
>>> It is not obvious to me that this code correctly replaces the code I wrote.

>>
>> If you believe that it doesn't correctly replace the code you wrote

>
> He hasn't said that this is what he believes. He is stating that your
> code does not *obviously* replace his correctly. It is up to you to
> prove it does, if you're going to say his is defective and you have come
> up with a replacement.


On the contrary: the code was four lines and used common C idioms. If it
isn't completely obvious to someone (or at the very least if they can't
mentally check in 10 seconds) what it does, then I don't believe they
know the language well enough to write it professionally.

(And if in turn you read my words to the letter, you'll notice that I
make no claim that Mr HeathField falls into this class of people: it is
my belief that he is being deliberately obtuse.)

 
Reply With Quote
 
Antoninus Twink
Guest
Posts: n/a
 
      10-08-2007
On 8 Oct 2007 at 12:22, Richard wrote:
>> char *dot_to_underscore(const char *s)
>> {
>> char *t, *u;
>> if(t=u=malloc(strlen(s)+1))
>> while(*u++=(*s=='.' ? s++, '_' : *s++));
>> return t;
>> }

>
> I would move the ++ part
>
> ,----
>| while(*u++=(*s=='.' ? '_' : *s))
>| s++;
> `----
>
> But yes, much nicer, easier to read and understand and possibly
> faster. And hopefully working :-;


That's indeed easier to read, though it's always satisfying to write a
loop with no body

 
Reply With Quote
 
Richard Heathfield
Guest
Posts: n/a
 
      10-08-2007
Richard said:

> Richard Heathfield <(E-Mail Removed)> writes:
>
>> Antoninus Twink said:
>>

<snip>
>>>
>>> char *dot_to_underscore(const char *s)
>>> {
>>> char *t, *u;
>>> if(t=u=malloc(strlen(s)+1))
>>> while(*u++=(*s=='.' ? s++, '_' : *s++));
>>> return t;
>>> }

>>
>> It is not obvious to me that this code correctly replaces the code I
>> wrote.

>
> Which part do you think doesn't "correctly" replace the code?


That's not what I said.

--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www. +rjh@
Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
"Usenet is a strange place" - dmr 29 July 1999
 
Reply With Quote
 
Richard
Guest
Posts: n/a
 
      10-08-2007
Philip Potter <(E-Mail Removed)> writes:

> Antoninus Twink wrote:
>> On 7 Oct 2007 at 22:55, Richard Heathfield wrote:
>>> It is not obvious to me that this code correctly replaces the code I wrote.

>>
>> If you believe that it doesn't correctly replace the code you wrote

>
> He hasn't said that this is what he believes. He is stating that your
> code does not *obviously* replace his correctly. It is up to you to


Don't be ridiculous. It was quite clear what RH meant. The shorter code
is FAR more obvious to me anyway - especially since it removes 2
unnecessary string operations which may or may not have caused hard to
spot behaviour. The shorter version is far, far more obvious in every
way. This is, of course, IMO abnd I am sure that a nOOb who cant read
the "?" operator and has no idea about post increment and pointer
dereferncing might have difficulties with the compact nature of the
second version. Personally I think it is easy enough to read at first
glance and the reduction in library calls makes it *easier* to
understand. having said that, I haven't tested or scrutinised the code
to ensure it reproduces in all cases exactly what the original code
produces.

> prove it does, if you're going to say his is defective and you have
> come up with a replacement.


No one said it was deceptive. heatjfield suggested it didn't do the same
thing "obviously". The original was, however long winded, overly white
spaced and unnecessarily inefficient IMO. Real code review if you
like. No one needs 20 lines for such a simple piece of code. This is C :
elegance and efficiency is everything. I suspect that it was overly
wordy for a reason since I would be surprised to see production quality
code like that.
 
Reply With Quote
 
Richard
Guest
Posts: n/a
 
      10-08-2007
"Joachim Schmitz" <(E-Mail Removed)> writes:

> "Antoninus Twink" <(E-Mail Removed)> schrieb im Newsbeitrag
> news:(E-Mail Removed)...
>> On 7 Oct 2007 at 22:55, Richard Heathfield wrote:
>>> Antoninus Twink said:
>>>> It is also written in a hard-to-read and clunky style.
>>>
>>> A matter of opinion. Which bit did you find hard to read?

>>
>> The function is a completely trivial one, yet I can't see it all at once
>> in my editor without scrolling!

> 20 lines don't fit your screen???


What a ridiculous argument.

It doesn't fit my source window in gdb for a start.

Why 20 lines when it can be 4 perfectly eligible, efficient lines?

Don't defend code just because of the source. The second version is
superior for production level code in at least two ways:

1) screen real estate (this IS important in the real world).
2) less library calls. less to worry about IMO.

>
> Bye, Jojo

 
Reply With Quote
 
Richard
Guest
Posts: n/a
 
      10-08-2007
Richard Heathfield <(E-Mail Removed)> writes:

> Philip Potter said:
>
>> Antoninus Twink wrote:
>>> On 7 Oct 2007 at 22:55, Richard Heathfield wrote:
>>>> It is not obvious to me that this code correctly replaces the code I
>>>> wrote.
>>>
>>> If you believe that it doesn't correctly replace the code you wrote

>>
>> He hasn't said that this is what he believes.

>
> I always knew you could read, Philip. Some other people, I'm not so
> sure about.


It was perfectly obvious what you meant. Word games don't even begin to
cover it.
 
Reply With Quote
 
Richard
Guest
Posts: n/a
 
      10-08-2007
Richard Heathfield <(E-Mail Removed)> writes:

> Richard said:
>
>> Richard Heathfield <(E-Mail Removed)> writes:
>>
>>> Antoninus Twink said:
>>>

> <snip>
>>>>
>>>> char *dot_to_underscore(const char *s)
>>>> {
>>>> char *t, *u;
>>>> if(t=u=malloc(strlen(s)+1))
>>>> while(*u++=(*s=='.' ? s++, '_' : *s++));
>>>> return t;
>>>> }
>>>
>>> It is not obvious to me that this code correctly replaces the code I
>>> wrote.

>>
>> Which part do you think doesn't "correctly" replace the code?

>
> That's not what I said.


Yes it is. If its not "obvious" then there is an issue. It is obvious to
me. Which part is not obvious to you?
 
Reply With Quote
 
Richard Heathfield
Guest
Posts: n/a
 
      10-08-2007
Richard said:

> Philip Potter <(E-Mail Removed)> writes:
>
>> Antoninus Twink wrote:
>>> On 7 Oct 2007 at 22:55, Richard Heathfield wrote:
>>>> It is not obvious to me that this code correctly replaces the code I
>>>> wrote.
>>>
>>> If you believe that it doesn't correctly replace the code you wrote

>>
>> He hasn't said that this is what he believes. He is stating that your
>> code does not *obviously* replace his correctly. It is up to you to

>
> Don't be ridiculous. It was quite clear what RH meant.


Well, I agree, but you appear to have misunderstood it nonetheless. I said
*precisely* what I meant to say, which is that it was not obvious to me
that this code correctly replaces the code I wrote.

<snip>

>> prove it does, if you're going to say his is defective and you have
>> come up with a replacement.

>
> No one said it was deceptive.


Nor did anyone say it was defective.

> heatjfield suggested it didn't do the same thing "obviously".


Well, I said it wasn't obvious to me that it does the same thing. Your
paraphrase of what I said is susceptible to interpretations that cannot
reasonably be drawn from my original phrasing.

<snip>

--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www. +rjh@
Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
"Usenet is a strange place" - dmr 29 July 1999
 
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
Designing fgetline - a perspective Richard Harter C Programming 48 11-25-2007 11:40 PM
Re: Bug/Gross InEfficiency in HeathField's fgetline program Dik T. Winter C Programming 2 11-07-2007 01:38 PM
Program inefficiency? hall.jeff@gmail.com Python 17 10-01-2007 04:48 PM
RE: Program inefficiency? Michael.Coll-Barth@VerizonWireless.com Python 6 10-01-2007 06:22 AM
Segfaulting when trying to create custom 'fgetline' function Jeff Rodriguez C Programming 4 11-17-2003 08:31 AM



Advertisments