Using function parameter as variable

Question

Is it bad or good practice or maybe undefined behavior to re-assign function parameter inside function?

Let me explain what I'm trying to do with an example, here the function:

void
gkUpdateTransforms(GkNode *node /* other params */) {
  GkNode *nodei;

  if (!(nodei = node->chld))
    return;

  do {
    /* do job */
    nodei = nodei->next;
  } while (nodei);
}

Alternative:

void
gkUpdateTransforms2(GkNode *node /* other params */) {

  /* node parameter is only used here to get chld, not anywhere else */
  if (!(node = node->chld))
    return;

  do {
    /* do job */
    node = node->next;
  } while (node);
}

I checked assembly output and it seems same, we don't need to declare a variable in second one. You may ask what if parameter type changed but same condition would be same for first one, because it also need to be updated.

EDIT: Parameters are pass-by-value, and my intention is not edit pointer itself

EDIT2: What about recursive functions? What would happen if gkUpdateTransforms2 was recursive? I'm confused because function will call itself but I think in every call, parameters will be different stack

The parameter is local to the function. Modifying *its* value is as good or bad as modifying any local. And it's only UB if you somehow make it take an indeterminate value. — StoryTeller - Unslander Monica, Dec 21 '17 at 08:25
It's perfectly fine to modify the parameter of a function inside the function. And it's not bad pratice at all. — Jabberwocky, Dec 21 '17 at 08:28
It's perfectly fine to do. Certainly not undefined behavior. — QuestionC, Dec 21 '17 at 08:29
"it's only UB if you somehow make it take an indeterminate value" I have no idea how to do that? But according to comments it seems I'm in safe area — recp, Dec 21 '17 at 08:31
There's no way to do that in a well-formed program. Either way, treat it like you would any local variable. You aren't doing anything bad. — StoryTeller - Unslander Monica, Dec 21 '17 at 08:32
You are essentially asking "how do functions work in C". If you understand why the code `int a=5; func(a); .... void func (int x) { x=0; }` does not change the value of `a`, then you should be able to answer this question yourself. If you don't understand that, well, you need to read a C programming book then. — Lundin, Dec 21 '17 at 08:47
As a side note, don't write code with assignment inside if statements like this, it is obfuscation. Instead do `if(node == NULL) { return ; } /* rest of the code */`. — Lundin, Dec 21 '17 at 08:48
@Lundin I'm not asking that question and I know x will not change, I'm asking if it is god/bad practice or UB — recp, Dec 21 '17 at 08:49
"if(node == NULL) { return ; }" would that make program faster e.g. helping branch prediction? — recp, Dec 21 '17 at 08:53
@recp nowadays the compiler takes care of this kind of optimisations for you. — Jabberwocky, Dec 21 '17 at 08:56

Lundin · Answer 1 · 2017-12-21T09:24:35.293

I have no idea why you think this would be undefined behavior - it is not. Mostly it is a matter of coding style, there's no obvious right or wrong.

Generally, it is good practice to regard parameters as immutable objects. It is useful to preserve an untouched copy of the input to the function. For that reason, it may be a good idea to use a local variable which is just a copy of the parameter. As you can see, this does not affect performance the slightest - the compiler will optimize the code.

However, it is not a big deal if you write to the parameters either. This is common practice too. Calling it bad practice to do so would be very pedantic.

Some pedantic coding styles make all function parameters const if they shouldn't be modified, but I personally think that's just obfuscation, which makes the code harder to read. In your case such pedantic style would be void gkUpdateTransforms(GkNode*const node). Not to be confused with const correctness, which is an universally good thing and not just a style matter.

However, there is something in your code which is definitely considered bad practice, and that is assignment inside conditions. Avoid this whenever possible, it is dangerous and makes the code harder to read. Most often there is no benefit.

The danger of mixing up = and == was noted early on in the history of C. To counter this, in the 1980s people came up with brain-damaged things like the "yoda conditions". Then around 1989 came Borland Turbo C which had a fancy warning feature "possible incorrect assignment". That was the death of the Yoda conditions, and compilers since then have warned against assignment in conditions.

Make sure that your current compiler gives a warning for this. That is, make sure not to use a worse compiler than Borland Turbo from 1989. Yes, there are worse compilers on the market.

(gcc gives "warning: suggest parentheses around assignment used as truth value")

I would write the code as

void gkUpdateTransforms(GkNode* node /* other params */) 
{
  if(node == NULL)
  {
    return ;
  }

  for(GkNode* i=node->chld; i!=NULL; i=i->next;)
  {
    /* do job */
  }
}

This is mostly stylistic changes to make the code more readable. It does not improve performance much.

I could bet you are talking about certain compilers in the automotive industry which by coincidence are being used exclusively for safety critical systems - what could possibly go wrong! ;) — Vroomfondel, Dec 21 '17 at 09:29
@Vroomfondel Indeed I was mostly referring to various embedded systems compilers. — Lundin, Dec 21 '17 at 09:34
You seem to be a notable exception - most of my customers take the tech in the SW tool corner as a given. Are you in Europe? — Vroomfondel, Dec 21 '17 at 09:41
some techniques/alternatives work by hacking compiler. I had to ensure that this is not hack, this is why I asked if it is UB or not, like @Vroomfondel answer next compilers may change this behavior (I don't now), this is why asked if it is bad/good practice... — recp, Dec 21 '17 at 10:21
also assigning variables in condition discussed here: https://stackoverflow.com/questions/151850/why-would-you-use-an-assignment-in-a-condition I don't think it make code less readable, I think compiler will interpret it as similar codes, but ok, I'll investigate it. PS: I'm already use parentheses around assignment — recp, Dec 21 '17 at 10:32
@recp The main reason not to use assignment inside conditions is because it is dangerous. There's the issue with == vs = but also it introduces an additional side effect, which may cause sequencing bugs if the same variable is used several times in the expression. That is, bugs like `if(i = arr[i++])`. — Lundin, Dec 21 '17 at 10:42
@Lundin I understand you, I don't / won't use something like this `if(i = arr[i++])`, I assure you :) I'm only using it for testing null pointers, that's it — recp, Dec 21 '17 at 10:48

score 2 · Answer 2 · answered Dec 21 '17 at 09:21

IMHO it is not exactly "bad" practice but it is worthwile to question oneself if there isn't a better way. About your analyzing the assembler output: it may serve as an interesting and educational look behind the curtain but you are ill advised to use this as an justification for optimization or worse, laziness in the source code. The next compiler or the next architecture may just render your musings completely invalid - my recommendation is to stay with Knuth here: "Instead of imagining that our main task is to instruct a computer what to do, let us concentrate rather on explaining to human beings what we want a computer to do.".

In your code I think the decision is 50:50 with no clear winner. I would deem the node-iterator a concept of its own, justifying a separate programming construct (which in our case is just a variable) but then again the function is so simple that we don't win much in terms of clarity for the next programmer looking at your code, so we can very well live with the second version. If your function starts to mutate and grow over time, this premise may become invalid and we were better off the first version. That said, I would code the first version like this:

void
gkUpdateTransforms(GkNode *node /* other params */) {    
  for (GkNode *nodei = node->chld; nodei != NULL; nodei = nodei->next) {
      /* do job */
  }
}

score 1 · Answer 3 · answered Dec 21 '17 at 08:36

1

This is well defined and a perfectly good way to implement this behaviour.

The reason you might see it as an issue is the common mistake of doing the following:

int func(object a) {
    modify a // only modifying copy, but user expects a to be modified

But in your case, you expect to make a copy of the pointer.

answered Dec 21 '17 at 08:36

Fantastic Mr Fox

32,495
27
95
175

I updated question to cover recursive functions, what about them? – recp Dec 21 '17 at 08:46
1

@recp concerning recursion: no problem, each call of the function gets it's own copy of the parameter. A function parameter is exactly the same thing as a local variable except that it has been initialized at the beginning of the function. – Jabberwocky Dec 21 '17 at 08:58
The use of function parameters in recursive call is a normally used method to create a value's stack. Because each function call create a new stacked copy of the value some programmers take advantage of this to create a pseudo stack that allows operations using current value against the value resulting from the successive recursive calls. – Frankie_C Dec 21 '17 at 09:12
@recp There's no difference for recursive functions - the parameters are local copies still. Now of course there's absolutely no need to use recursion here, the only thing you get from recursion here is: slower execution, more RAM use, less readable code, danger of stack overflows. – Lundin Dec 21 '17 at 09:21

score 1 · Answer 4 · answered Dec 21 '17 at 08:38

1

As long as it's passed by value, it can be safely treated as any other local variable. Not a bad practice in this scenario, and not undefined behaviour either.

answered Dec 21 '17 at 08:38

Brishna Batool

445
3
15

Using function parameter as variable

4 Answers4