Should function argument be changed in function body?

T

Tim Rentsch

CBFalconer said:
And I would say a better guide is to make the answers be YES.

Do you mean to say that every function body that you write is under 10
lines, simple and clearly/obviously correct (including to other
readers), and highly unlikely to have to change in the foreseeable
future?


[snip]

All of this is basically just another style war. We have expressed
opinions, together with some justifications for such, and thus have
divided the world into non-cretins and cretins (where obviously
cretins disagree with me :).

In my experience style wars tend to happen when two parties simply
express little more than their own opinions; don't offer constructive
reasoning; refuse to engage in useful dialog; don't examine both the
costs and benefits on both sides; and don't try to identify when
certain practices make sense and when they don't.

I don't like style wars so I usually try to avoid people who exhibit
those behaviors, and also try not to exhibit them myself.
 
M

Markus.Elfring

CBFalconer said:
And I would say a better guide is to make the answers be YES. The
sort of usages you listed earlier in the snipped portion come under
the heading of "needed later" to me. While something like:

void foo(T param) {
T saveparam = param;

/* machinations */
}

only requires one extra line, it does require extra code.
Certainly that code may get optimized away, and very likely will if
saveparam is never used. Meanwhile it is just one more thing to
confuse a maintainer, and one more expansion of the source code.
It is also one more place for an optimizer to become confused.

Do all compilers optimise the unnecessary copying of function
parameters away?
Is a coding guideline discussed here that has got no measurable effect
on the generated machine instructions?

Regards,
Markus
 
C

CBFalconer

Tim said:
Do you mean to say that every function body that you write is
under 10 lines, simple and clearly/obviously correct (including
to other readers), and highly unlikely to have to change in the
foreseeable future?

I don't always succeed, but yes, those are objectives. :) You
will have to judge the "including to others" portion yourself.
Feel free to root around in:

<http://cbfalconer.home.att.net/download/>
 
C

Chris Torek

Do all compilers optimise the unnecessary copying of function
parameters away?

Clearly the answer to this is "no", because there are some compilers
that do no optimization (either because they are not asked to, as
with gcc -O0, or because they simply do not do it).

One could instead ask: "do all good compilers optimize this away
when asked?" But this requires defining "good" :) -- we could
simply claim that a "good" compiler *must* do so, and create a
tautology.

I have a simple technique that I prefer when dealing with functions
that, for whatever reason, need to take a parameter of the "wrong
type" (and then convert it to the right type) and/or save the
initial value of a parameter for some reason. I name the incoming
value "foo0", stealing the subscript-0 notation from mathematical
sequences:

X = <initial value>
0

X = <formula based on X[i-1]>
i

So I write:

void foo(T0 bar0) {
T bar = bar0;
... code ...
}

If T0 and T are the same type, this simply saves the original
value; if T0 and T are different types -- as is the case when a
callback function receives a "void *" parameter -- it also
adjusts the type and value of the parameter as needed:

/*
* We get called through a function pointer, and passed
* a pointer to our data structure as converted to "void *".
* We must return the value to be given to us on the next
* callback, or NULL to indicate that we should not be called
* again.
*/
void *some_callback(void *sp0) {
struct somestruct *sp = sp0;

sp->ncalls++;
do_something(sp->other);
sp->third = some_op(sp->fourth);
return sp->freeme ? (free(sp), NULL) : sp0;
}

for instance.
 
M

Mark

CBFalconer said:
I don't always succeed, but yes, those are objectives. :) You
will have to judge the "including to others" portion yourself.
Feel free to root around in:

<http://cbfalconer.home.att.net/download/>

uncmntc.zip - (picked at random)All with the exception of main.
Simple? Definitely. Clearly and obviously correct? No.
It was actually pretty obvious that it would not work properly :)

As for style... PICK ONE!
I can understand why one puts the constant first in if statements
(in case you accidentally put = in place of == blah blah blah...
must be too hard to just look at the damn code when you type it!)
but you go 'both ways' on back to back lines! (74/75)
That depends on whether or not you choose to fix your bugs...
What would your utility do if it encountered a line such as:
printf("What\\" /* embedded string */ "%s joke!\n", /* doh */ "a
/*#(K1N5");

Oh, it will strip something alright, but probably not what you'd expect ;)

Regards,
Mark
 
C

CBFalconer

Mark said:
.... snip ...

uncmntc.zip - (picked at random)

[doubtless because of its gargantuan size :). Is that random?]
All with the exception of main.

Simple? Definitely. Clearly and obviously correct? No.
It was actually pretty obvious that it would not work properly :)

Wasn't to me at the time. :-(
As for style... PICK ONE!
I can understand why one puts the constant first in if statements
(in case you accidentally put = in place of == blah blah blah...
must be too hard to just look at the damn code when you type it!)
but you go 'both ways' on back to back lines! (74/75)

sad but true.
That depends on whether or not you choose to fix your bugs...
What would your utility do if it encountered a line such as:
printf("What\\" /* embedded string */ "%s joke!\n", /* doh */ "a
/*#(K1N5");

Oh, it will strip something alright, but probably not what you'd expect ;)

Well, you picked up that mishandling of escapes in echostring. At
least it was organized so that you could find that bug :). I
don't think it is getting heavily used though, since I have had no
other bug reports in 3 or so years.

Where should I turn in my certificate of infallibility?

In all seriousness, I consider the rapidity with which you could
spot a fairly obscure bug as vindication of the style.
 
T

Tim Rentsch

CBFalconer said:
Tim said:
CBFalconer said:
Tim Rentsch wrote:
[restoring snipped portion]

To be pragmatic, a rule that says one should *never* modify a
parameter variable might be considered a bit dogmatic. But as a
guideline it almost certainly helps more than it hurts. To say
that another way, during code review the writer of the code should
have the burden of defending a function body that modifies a
parameter. As a reviewer, my questions would be something like
these:

[end restoring snipped portion]

1. Is the function body small (under 10 lines, say)?
2. Is the code simple, and clearly and obviously correct?
3. Is the code highly unlikely to have to change in the
foreseeable future?

If the answers to any of the questions is NO, then it's almost
certainly better to follow the guideline and not modify any of
the parameter variables.

And I would say a better guide is to make the answers be YES.

Do you mean to say that every function body that you write is
under 10 lines, simple and clearly/obviously correct (including
to other readers), and highly unlikely to have to change in the
foreseeable future?

I don't always succeed, but yes, those are objectives. :)

I don't know whether you missed reading the snipped paragraph or just
oversnipped. The questions are questions to be asked during code
review. They are asked about the code, not about the person who wrote
the code; effort or intention don't enter into the considerations.

You
will have to judge the "including to others" portion yourself.
Feel free to root around in:

<http://cbfalconer.home.att.net/download/>

I looked at id2id-20.zip, files hashlib.c and id2id.c. The median
function body length was about 12 lines in both cases, the average
function body length was a little higher, between 14 and 16 lines.
Although those numbers may be admirably low, they don't meet the
guideline mentioned.

To get back to the original topic: I don't believe the code I looked
at makes a good case that modifying function parameters should, as a
general rule, be thought of as good style. At the very least, it
should be immediately obvious which parameters are modified and which
are not. If a development team really wanted the freedom to modify a
function parameter on any function, then it would be good practice to
attach 'const' to any parameters that are not modified. There doesn't
seem to be much upside to allowing function parameters to be modified;
conversely, following a guideline that function parameters not be
modified has relatively low costs and fairly clear benefits. So even
though there may be specific exceptions, as a general development
practice discouraging modification of function parameters seems to
offer a good ROI.
 
S

Steve Summit

Tim said:
There doesn't seem to be much upside to allowing function parameters
to be modified; conversely, following a guideline that function
parameters not be modified has relatively low costs and fairly
clear benefits.

Can you name some of those benefits? They're not clear to me.

(I agree that modifying function parameters is not something
you'd want to do every day, but sometimes it's convenient -- more
so that introducing a local variable to hold a modifiable copy --
and I don't understand the predilection in some circles for
mandating that parameters not be modified.)

Steve Summit
(e-mail address removed)
 
G

Gordon Burditt

There doesn't seem to be much upside to allowing function parameters
Can you name some of those benefits? They're not clear to me.

(1) The original value is available in a debugger. It gets especially
confusing in stack traces, where the value printed for the call is
sometimes the original call value and is sometimes the modified
value, depending on compiler options.

I quit modifying function parameters after burning myself a couple
of times because of this (in code I originally wrote).

(2) The original value is sometimes wanted after original coding
for logging or debugging. I've managed to waste a lot of time when
I assumed that what was being printed was the original value, and
it wasn't.

(3) It can save hours of arguing with pedants who argue, among other
things, that goto is evil and printf("Go to the store and buy some
beer.\n"); is a use of goto in English.
(I agree that modifying function parameters is not something
you'd want to do every day, but sometimes it's convenient -- more
so that introducing a local variable to hold a modifiable copy --
and I don't understand the predilection in some circles for
mandating that parameters not be modified.)

I don't think this issue rises to the level of having to call
in the coding style police.
Steve Summit
(e-mail address removed)

Gordon L. Burditt
 
O

Old Wolf

Gordon said:
(1) The original value is available in a debugger. It gets especially
confusing in stack traces, where the value printed for the call is
sometimes the original call value and is sometimes the modified
value, depending on compiler options.
I quit modifying function parameters after burning myself a couple
of times because of this (in code I originally wrote).

I regularly debug with an IDE, and have never had any problem
inspecting the values of parameters, even if they've changed.
Once you've passed the opening { , those parameters are no
different to any other local variables.
(2) The original value is sometimes wanted after original coding
for logging or debugging. I've managed to waste a lot of time when
I assumed that what was being printed was the original value, and
it wasn't.

You can avoid this by declaring the parameters as const
(in the function definition -- you don't have to do it in
the prototype).

Also there's the argument that if you can't tell in a glance
whether the value has been modified, either your function is
too large or your variable name is too short (not that I
subscribe much to this argument, but it's out there).
(3) It can save hours of arguing with pedants who argue, among other
things, that goto is evil and printf("Go to the store and buy some
beer.\n"); is a use of goto in English.

Come on, don't you enjoy these discussions ;)
 
G

Gordon Burditt

There doesn't seem to be much upside to allowing function parameters
I regularly debug with an IDE, and have never had any problem
inspecting the values of parameters, even if they've changed.

Well, when I look at a stack trace, I expect those values to
be the *ORIGINAL* values at the time of the call (since a lot
of messups happen because garbage was passed in to the function).
With parameters being modified, sometimes they are the original
values (compilers put parameter in a register and used it) and
sometimes they aren't (compiler left variable on the stack frame).
I don't know whether I ever did figure out a good way to determine
whether the stack trace was giving the original or the current value,
other than directly testing it by setting a breakpoint.

If I want the current value of the parameter, that isn't a problem.
But if I am, for example, traversing a linked list, it's likely to
have NULL or garbage when it screws up, and I'd like to know how I
got there.
Once you've passed the opening { , those parameters are no
different to any other local variables.

You can avoid this by declaring the parameters as const
(in the function definition -- you don't have to do it in
the prototype).

In other words, DON'T modify parameters. const alone won't do
that (just prevent it from compiling if you try).
Also there's the argument that if you can't tell in a glance
whether the value has been modified, either your function is
too large or your variable name is too short (not that I
subscribe much to this argument, but it's out there).

Yes, I remember this argument, especially applied to a function to
print a blank form. It took one argument (the file descriptor to
put the form on, which varied from stdout (using a printing terminal)
to one gotten from popen("lpr", "w"). ). In theory, it could have
been written with one fputs() call with a very long quoted string,
but it was desirable to make the code look somewhat like the way
the form would print. So it had roughly one fputs() per line printed
on the form. It made no attempt to pre-fill-in blanks. The idea
here is that these forms were supposed to be filled out, then
entered, and after you entered one, it gave you another blank form.
This was before everyone had a laptop with wireless access to fill
these out online. Besides, you get fewer poll results if you are
trying to balance a laptop on the back of the person you're polling
while both of you are outside on the sidewalk.
Come on, don't you enjoy these discussions ;)

I can walk away from USENET. Pedants at work are harder to walk
away from, especially if one of them is your boss.

Gordon L. Burditt
 
C

Chris Torek

[On reasons to leave the formal parameter variable unchanged in the
function body...]

(1) The original value is available in a debugger. It gets especially
confusing in stack traces, where the value printed for the call is
sometimes the original call value and is sometimes the modified
value, depending on compiler options.

I quit modifying function parameters after burning myself a couple
of times because of this (in code I originally wrote).

Not modifying the formal parameter will not save you from this
fate:

void f(int x0, int limit) {
int x;

for (x = x0; x < limit; x++)
do_something(x);
}

may compile to identical machine code as the same code with "x0"
changed to "x", and the now-redundant declaration for "x" removed.
(Indeed, I would expect this on machines that pass parameters in
registers.)

If you really need x0 saved, you would have to use it:

int f(int x0, int limit) {
int x;

for (x = x0; x < limit; x++)
do_something(x);
return x0;
}

but even then, I have had compilers decide that %i0 should be used
for the loop, and copied into %l1 at function entry, with the return
sequence being compiled as:

ret
restore %g0, %l1, %o0

In other words, the compiler cleverly rewrote my code as if it
read instead:

int f(int x, int limit) {
int x0 = x;

for (; x < limit; x++)
do_something(x);
return x0;
}
(2) The original value is sometimes wanted after original coding
for logging or debugging.

For this, I like the "x0" convention exhibited above (preferably
as the formal parameter name, rather than copied in the function
entry, but either one suffices).
(3) It can save hours of arguing with pedants who argue, among other
things, that goto is evil and printf("Go to the store and buy some
beer.\n"); is a use of goto in English.

Indeed. :)
 
C

CBFalconer

Tim said:
.... snip ...
general rule, be thought of as good style. At the very least, it
should be immediately obvious which parameters are modified and which
are not. If a development team really wanted the freedom to modify a
function parameter on any function, then it would be good practice to
attach 'const' to any parameters that are not modified. There doesn't
seem to be much upside to allowing function parameters to be modified;
.... snip ...

On the contrary, hanging extraneous 'const' on the parameters would
impose an extra anomalous connection between code modules. Without
it a future revisor can go ahead and confidently make the changes
he needs. The only real reason for const is to protect things
passed in via pointers, i.e. by reference in other languages.
Similarly a coder should strive to mark local functions as static.
 
S

Steve Summit

Gordon said:
[I had written:]
Can you name some of those benefits? They're not clear to me.

(1) The original value is available in a debugger. It gets especially
confusing in stack traces...

Ah, that problem. Now that you mention it, I've been burned by
that, too.
(2) The original value is sometimes wanted after original coding
for logging or debugging.

Sure -- but that's a reason you might not want to do it under a
certain circumstance, not a reason for a blanket ban!
 
T

Tim Rentsch

Can you name some of those benefits? They're not clear to me.

Sure. In a previous post I listed some specific cases where it's
useful to have the initial value available (quoting):

showing the value with printf for some tracing
writing an assertion relating the function's output and input
viewing the value while debugging

All of these uses (not to mention some others) tend to show up
later in the development cycle as much as they do earlier when
the code is first written. It often isn't possible to
anticipate on first writing when the original value will
subsequently be needed.

[End of quoting.]

There's also a different kind of benefit having to do with
comprehensibility. For example, in long functions, it's
usually better to initialize a loop variable right before
the loop: because the initialization is right there, it's
easy to see what it is, and know that the variable hasn't
been inadvertently set to the wrong thing. We can't do
that with a parameter -- it had to be initialized at the
beginning. Compare

t = n*3 + 7;

...

while( t-- ){
...
}

with

for( t = n*3 + 7; t--; ){
...
}

The second is better (I claim), because everything we
have to read for the loop is right there at the loop.
We don't always have this choice if parameters are used
in loops, because of how they are initialized.

Another example of a comprehensibility benefit is where
on the spectrum of imperative vs functional a piece of
code is. The more there is state that changes, and the
less there is state that doesn't change, generally the
harder we have to work to understand a given piece of
code. For example, compare

size_t
length_of_string( const char *s ){
size_t n = 0;

while( *s ) s++, n++;
return n;
}

with

size_t
length_of_string( const char *s ){
size_t n = 0;

while( s[n] ) n++;
return n;
}

I admit the difference between these functions isn't very big,
and in a code review I'd probably be ok with either one. But
the second one is a little easier to understand. I can see at a
glance that 'n' holds the index of the first null character in
the string 's'. Of course, I can see that in the first function
also, but there's a little more mental effort involved. You
see what I mean?

Summarizing the two examples above: a benefit of keeping
parameters unchanged is that it allows or encourages other
good development practices.

(I agree that modifying function parameters is not something
you'd want to do every day, but sometimes it's convenient -- more
so that introducing a local variable to hold a modifiable copy --
and I don't understand the predilection in some circles for
mandating that parameters not be modified.)

I expect we're not really that far apart on this. Let me offer
an analogy. Usually it's better for (non-const) variables to be
initialized subsequent to their declaration. Not always better,
not hugely better, but usually better. Every so often I am
tempted to "break the rule" and write initializing declarations.
When I give in to that temptation, probably 999 times out of
1000 everything works out fine. But every now and then I get
bitten by it. That's the thing: when I break the rule,
sometimes I get bitten; but when I follow the rule, I never do.

I suspect people who are strong advocates of never modifying
parameters do so largely for that reason - if one follows the
rule, one never gets bitten.

So if you want to make exceptions now and then, that's ok, just
be sure you bring your venom kit with you. :)
 
C

Chris Croughton

The second is better (I claim), because everything we
have to read for the loop is right there at the loop.
We don't always have this choice if parameters are used
in loops, because of how they are initialized.

I would generally agree with that, except in very small functions.
Another example of a comprehensibility benefit is where
on the spectrum of imperative vs functional a piece of
code is. The more there is state that changes, and the
less there is state that doesn't change, generally the
harder we have to work to understand a given piece of
code. For example, compare

size_t
length_of_string( const char *s ){
size_t n = 0;

while( *s ) s++, n++;
return n;
}

I would write it as:

size_t
length_of_string(const char *s)
{
size_t n = 0;
while (*s++) n++;
return n;
}

In general, I dislike the comma operator, it's too easily misread. In
cases where the result isn't needed, such as yours, I prefer to treat
them as statements and put them in a block.
with

size_t
length_of_string( const char *s ){
size_t n = 0;

while( s[n] ) n++;
return n;
}
I admit the difference between these functions isn't very big,
and in a code review I'd probably be ok with either one. But
the second one is a little easier to understand. I can see at a
glance that 'n' holds the index of the first null character in
the string 's'.

It does, but I find that less obvious (what I want is the length of the
string, the position of the null character happens to be the same with C
strings but it is not a trivial equivalence).
Of course, I can see that in the first function
also, but there's a little more mental effort involved. You
see what I mean?

Actually, I feel the opposite.
Summarizing the two examples above: a benefit of keeping
parameters unchanged is that it allows or encourages other
good development practices.

Sometimes. It can also obscure things. For instance, take a function
similar to strcpy:

/**
* Copy a string, returning a pointer to the terminating null
* character of the destination.
*/

char *copy_string(char *dst, const char *src)
{
while (*src)
*dst++ = *src++;
return dst;
}

Doing that without modifying the parameters is more messy.
So if you want to make exceptions now and then, that's ok, just
be sure you bring your venom kit with you. :)

Indeed.

I do think that the general rules outlined several posts ago are good, a
function which modifies its parameters should be short and 'obviously'
correct, and should be trivial enough that it is unlikely to need
changing such that modifying the parameters is a problem. As in the
case of my copy_string() above, where the interface defines that only
the modified version of the destination pointer will ever be needed.

Chris C
 
C

CBFalconer

Chris said:
I would generally agree with that, except in very small functions.


I would write it as:

size_t
length_of_string(const char *s)
{
size_t n = 0;
while (*s++) n++;
return n;
}

In general, I dislike the comma operator, it's too easily misread.
In cases where the result isn't needed, such as yours, I prefer to
treat them as statements and put them in a block.

Of course, after all the fuss, this is a function that can be
improved by not altering the input parameter.

size_t length_of_string(const char *s) {
const char *t = s;

while (*t++) continue;
return t - s;
}

(and I will live with the possibility that ptr_diff is too small)
 
T

Tim Rentsch

CBFalconer said:
... snip ...

On the contrary, hanging extraneous 'const' on the parameters would
impose an extra anomalous connection between code modules.

What connection would that be? Declaring a parameter 'const'
(the parameter itself, not something it points to) doesn't
impose any burden on any call site.

Without
it a future revisor can go ahead and confidently make the changes
he needs. The only real reason for const is to protect things
passed in via pointers, i.e. by reference in other languages.
Similarly a coder should strive to mark local functions as static.

If one is in the habit of modifying parameter variables, the value of
'const' on a parameter is the same as that for other function-local
variables -- to guarantee that the variable is not modified after
getting its initial value. That in turn reduces cognitive load on
someone reading the code, knowing that the compiler would squawk
if the 'const' prohibition were violated.
 
T

Tim Rentsch

Chris Croughton said:
Another example of a comprehensibility benefit is where
on the spectrum of imperative vs functional a piece of
code is. The more there is state that changes, and the
less there is state that doesn't change, generally the
harder we have to work to understand a given piece of
code. For example, compare

[...snip my code, leaving his nearly identical version...]

size_t
length_of_string(const char *s)
{
size_t n = 0;
while (*s++) n++;
return n;
}

[snip]
with

size_t
length_of_string( const char *s ){
size_t n = 0;

while( s[n] ) n++;
return n;
}
I admit the difference between these functions isn't very big,
and in a code review I'd probably be ok with either one. But
the second one is a little easier to understand. I can see at a
glance that 'n' holds the index of the first null character in
the string 's'.

It does, but I find that less obvious (what I want is the length of the
string, the position of the null character happens to be the same with C
strings but it is not a trivial equivalence).
Of course, I can see that in the first function
also, but there's a little more mental effort involved. You
see what I mean?

Actually, I feel the opposite.

I know where you're coming from. What code seems "natural" to
people certainly varies from person to person. Even though this
example wasn't the best, however, I hope you can see the
principle that I'm trying to illustrate. Consider two versions
of matrix multiplication, for example - one written out using
arrays and normal index calculations, the other using pointer
variables being changed and updated with only plus and minus.
The "less imperative" version that uses arrays and just simple
loops with i,j,k index variables is more likely to be easy
to understand.

Sometimes. It can also obscure things. For instance, take a function
similar to strcpy:

/**
* Copy a string, returning a pointer to the terminating null
* character of the destination.
*/

char *copy_string(char *dst, const char *src)
{
while (*src)
*dst++ = *src++;
return dst;
}

Doing that without modifying the parameters is more messy.

(I'm assuming you meant to write slightly different code that
returned the same value but caused the destination string to be
null-terminated. Please read the comments below as if that
code were written above.)

I think we're in agreement that there are cases (the above
probably being one) where modifying the paramters yields a
better-overall function body. Where we may disagree is what
the magnitude of the difference is. The function body above
re-written to use locals rather than modifying its parameters
might be a line or two longer, but I don't think I'd say that
code would obscure things. Perhaps I'm just more used to a
style that almost never modifies parameters.

Indeed.

I do think that the general rules outlined several posts ago are good, a
function which modifies its parameters should be short and 'obviously'
correct, and should be trivial enough that it is unlikely to need
changing such that modifying the parameters is a problem. As in the
case of my copy_string() above, where the interface defines that only
the modified version of the destination pointer will ever be needed.

In practice it sounds like our views are actually pretty
similar. I appreciate the comments.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,734
Messages
2,569,441
Members
44,832
Latest member
GlennSmall

Latest Threads

Top