Assignment in condition of if goes wrong

Question

I'm dynamically allocating memory for a struct with this line of code:

if (DrinkMachine = (Drink_Machine*)malloc(sizeof(DrinkMachine) * size) == NULL)
    return(NULL);

I've also tried:

if (DrinkMachine = (Drink_Machine*)malloc(sizeof(DrinkMachine[size])) == NULL)
    return(NULL);

Now, due to an error in my code (somewhere), it's failing to dynamically allocate the memory and the pointer is indeed NULL. However, it doesn't return NULL, or otherwise attempt to enter the if statement, even though it should be true. What gives?

Edit: Additional Information. Each item in my struct shows "Unable to read memory" in the watch, when I step through the debugger. Don't know if this helps anyone, but I figured I'd add it to the question.

Edit2: So This was answered I just wanted to make it visible in an edit.

if ((DrinkMachine = (Drink_Machine*)malloc(sizeof(Drink_Machine) * size)) == NULL)
    return NULL;

This is the right way to type this. Note, I also edited the code to properly show Drink_Machine in the sizeof(). Not including that was a typo. Others have stated that apparently, it's smarter to just declare the code separately then test the pointer in the next statement. (and one typo corrected)

Thanks to everyone who took the time to answer this question!

@PaulRooney, one is a (poorly chosen) variable name, and the other is a type. no mistake there. — StoryTeller - Unslander Monica, Oct 09 '16 at 23:15
Also, [don't cast the return type of malloc](http://stackoverflow.com/a/605858/817643) — StoryTeller - Unslander Monica, Oct 09 '16 at 23:16
If you had written `DrinkMachine = malloc( ... ); if (NULL == DrinkMachine) {...}`, you wouldn't have had this problem. Was saving that one line of code worth that? — Andrew Henle, Oct 09 '16 at 23:21
@self, having Visual Studio is a poor reason to do anything, and in particular for writing in what is considered more & more to be bad style. — StoryTeller - Unslander Monica, Oct 09 '16 at 23:23
Sorry, I don't know a ton about coding. I just thought this was a nifty way to write my code in fewer lines while also returning NULL when it fails. So a couple of you mentioned that this works in visual studio (which I'm using), does this not work in other IDEs like Eclipse? — Jonathan Martin, Oct 09 '16 at 23:32
It is valid c (on any compiler), the discussion was about the cast, not the assignment + check (which you just wrote wrong) — StoryTeller - Unslander Monica, Oct 09 '16 at 23:36
Please don't edit the solution into the Question, it makes it confusing for new readers. Instead accept one of the posted Answers, or post your own Answer if none are suitable. (Currently you accepted an answer that is wrong) — M.M, Oct 10 '16 at 00:05
`DrinkMachine` is a pointer, so it's very likely that you want to take `sizeof(*DrinkMachine)`, because it looks like you want to allocate an array of `Drink_Machine`s. — FRob, Oct 10 '16 at 00:07

Ryan · Accepted Answer · 2016-10-10T00:41:37.013

3

You need to wrap the assignment in parenthesis in order to force precedence.

if((DrinkMachine = (Drink_Machine *)malloc(sizeof(Drink_Machine) * size)) == NULL) {
    /* DrinkMachine is NULL */
}

edited Oct 10 '16 at 00:41

answered Oct 09 '16 at 23:12

Ryan

14,392
8
62
102

this is not correct, the allocated memory will be the size of a pointer, not the size of a 'DrinkMachine' object. – user3629249 Oct 10 '16 at 16:43
The `Drink_Machine` is the object, I'm assuming it is a typedef – Ryan Oct 10 '16 at 17:59

score 2 · Answer 2 · answered Oct 09 '16 at 23:16

2

== has higher precedence than =.

So the == is computed first (resulting in 0 or 1) and that's assigned to your pointer.

answered Oct 09 '16 at 23:16

Andrew Henle

32,625
3
24
56

And some people continue to wonder why I think assignments inside conditional clauses are a **BAD** idea... – Andrew Henle Oct 09 '16 at 23:19
Yes, it took me a long time to actually write it out, I had to fire open an editor with auto parens because it was hard to parse – Ryan Oct 09 '16 at 23:24
1

@self Yep. I've been coding in C for the better part of 25 years (ouch!), and I think dealing with operator precedence is wasted effort - I'd rather concentrate on *what* happens and not waste hours on the *order*. IMO the "assignment in conditional clause" style comes from textbooks where saving lines of code matters - a few pages on a few tens of thousands of textbooks will help pay for someone's swimming pool. But line-of-code count is irrelevant in the real world. – Andrew Henle Oct 09 '16 at 23:31
^-- This is the case here. I've always been told to condense code if possible. Is this not the norm? – Jonathan Martin Oct 09 '16 at 23:37
3

@JonathanMartin We prefer *readable*, *maintainable* code. There's no reason to slam stuff together on the same line unless you're competing in the Obfuscated C contest http://www.ioccc.org/. – 3Dave Oct 09 '16 at 23:39
@JonathanMartin What good does condensing code do you? You don't store source code on memory-limited platforms - you store the binaries. The best for code is that it's *clear* and *error-free*. Condensing it makes the *error-free* harder - and it's already hard enough to write bug-free code. – Andrew Henle Oct 09 '16 at 23:40
That's good to know. Yeah, when I was debugging another error, (I did sizeof(DrinkMachine) instead of sizeof(Drink_Machine), I just pasted the code block on its own line to make it easier to read. So I can certainly appreciate why it's better coding to type it that way. – Jonathan Martin Oct 09 '16 at 23:42
@JonathanMartin Also, the human mind can only handle a few things going on at one time - the research I've seen places that limit at 3-7 "things". Putting 4 or 5 "things" at once into one line makes code **MUCH** harder to understand. One example: http://www.livescience.com/2493-mind-limit-4.html Write **SIMPLE** code. Brian Kernighan wrote "Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?" – Andrew Henle Oct 09 '16 at 23:42

user3629249 · Answer 3 · 2016-10-10T02:05:32.333

regarding this line:

if (DrinkMachine = (Drink_Machine*)malloc(sizeof(DrinkMachine) * size) == NULL)
return(NULL);

1) do not cast the returned value from any of the heap memory allocation functions (malloc, calloc, realloc)

2) the variable DrinkMachine must be a pointer. so the expression: sizeof(DrinkMachine) will return the size of a pointer NOT the size of a Drink_Machine.

3) the syntax is also incorrect. it should be:

if ( (DrinkMachine = malloc(sizeof(*DrinkMachine) * size) ) == NULL)
return(NULL);

4) when a system function returns an error indication, the recognition of that error should be followed by: (in this case)

{
    perror( "malloc for DrinkMachine failed" );
    // the code has failed, so cleanup and exit
    cleanup();
    exit( EXIT_FAILURE );
}

Assignment in condition of if goes wrong

3 Answers3