7

I have the following test code:

#include <cstdint>
#include <cassert>

enum class Result : std::uint32_t {SUCCESS = 0, INSUCCESS = 1};

void* func(Result& result)
{
    // works great
    /*
    result = Result::INSUCCESS;
    return NULL;
    */

    // error: invalid conversion from ‘long int’ to ‘void*’ [-fpermissive]
    /*
    return result = Result::INSUCCESS, NULL;
    */

    // compiles, but <result> is not set???
    return result = Result::INSUCCESS, nullptr;
}

void testReturnWithSideEffects()
{
    Result result = Result::SUCCESS;
    func(result);
    assert(result == Result::INSUCCESS);
}

There are 2 questions here but I'm mostly interested in the second:

Why is result not set?

Edit: Thanks to everyone for confirming this. The work-around that I have decided to use is to replace:

return result = Result::INSUCCESS, nullptr;

with the following:

return result = Result::INSUCCESS, (void*)NULL;

Additional note: of course my production scenario is with another pointer type (not void*) but I simplified for illustrative purposes.

Another note: from the workaround you can tell that there's something fishy going on with that nullptr. I'm guessing that the example line which doesn't compile, should actually compile, and these 2 matters are probably related somehow.

And a 3rd and final note, to those who outlined the "trickery" or "unreadability" of my code: readability is largely a subjective matter, for instance I can argue that a shorthand like this can make the code more structured which can actually help spot defects.

Zoe
  • 27,060
  • 21
  • 118
  • 148
haelix
  • 4,245
  • 4
  • 34
  • 56
  • Note: I'm using gcc 4.6.3 with -O0 -ggdb -std=c++0x – haelix May 09 '14 at 11:59
  • 6
    This probably compiles as `return (result = (Result::INSUCCESS, nullptr));`, so `result == nullptr`. Looks like you are trying to be too "tricky". Just do it in two lines. – crashmstr May 09 '14 at 11:59
  • 1
    *works for me* (clang++ --version `Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)`) -- Also gcc 4.8.1 in ideone. – David Rodríguez - dribeas May 09 '14 at 12:00
  • 1
    *works for me too* on gcc 4.9.0 (Except I get compile warning `warning: elaborated-type-specifier for a scoped enum must not use the 'class' keyword`). – Felix Glas May 09 '14 at 12:06
  • 7
    If I were you, I'd avoid such "cleverness" at all cost. Always prefer readability over "cute" (to use Herb Sutter's words) solutions. – lethal-guitar May 09 '14 at 12:07
  • Also, I'm sure you cut code out, but it seems like you don't even care about the return value, so not sure even why you have/need this code (i.e. returning a `void*`). – crashmstr May 09 '14 at 12:09
  • Works on 4.8 as well (after correcting the includes to add cstdint... also it shouldn't be stddef.h but cstddef). – Joe May 09 '14 at 12:09
  • 1
    BTW about the second variant: C++11 *only* allows literal 0 or nullptr to initialize pointers to zero, not an integer-valued expression that evaluates to 0. – Joe May 09 '14 at 12:11
  • @DavidRodríguez-dribeas I am able to reproduce using `gcc 4.6` see it [live](http://coliru.stacked-crooked.com/a/93924ea518bf1ee0) ... very odd. – Shafik Yaghmour May 09 '14 at 12:15
  • 1
    I fixed the code, `uint32_t` was undefined. Clang throws the correct error, GCC fails miserably and starts complaining cryptically about the enum declaration/definition. – rubenvb May 09 '14 at 12:15
  • 5
    @crashmstr, if it does get compiled like that, it is a compiler bug. Comma operator has lower precedence than assignment. – eerorika May 09 '14 at 12:28
  • 2
    @haelix: Regarding the readability. How often have you seen something like that in other code (not yours)? Readability is often influenced by previous experience. Anything that is *different* is inherently harder to read just because it is surprising. Add on top of that the fact that while everyone should know all the details in the language, not that many people are familiar with the *comma* operator... – David Rodríguez - dribeas May 09 '14 at 14:26
  • True. Personally I'm often surprised how few people know the comma operator and how many consider it almost a taboo to use it (which of the two came first?) – Joe May 09 '14 at 15:06
  • @Joe *"BTW about the second variant..."* That's not entirely true. A null pointer constant is an integral constant expression evaluating to `0` (or `std::nullptr_t`). The assignment makes the `a = x, 0` not a constant expression, therefore the second example fails to compile. – dyp May 09 '14 at 15:08
  • @DavidRodríguez-dribeas Simple question, would _you_ be surprised, in a negative way, to find such return statements in the code of your work colleague? Would _you_ consider it harder to read? – haelix May 10 '14 at 09:43
  • 2
    @haelix i wiuld not let that throught a code review, if that answers your question. I would be surprised (it is an uncommon unexpected construct) even if I know what it does. And you don't know how much a junior programmer that has to maintain this down the road will know. I limit 'expert constructs' to situations where there's no alternative, or where the advantage grants the complexity. If you waste your smarts writing code, you won't have any left to maintain/debug it – David Rodríguez - dribeas May 10 '14 at 14:46
  • @David R. - I'm suspecting that 'expert constructs' like you call them, are often just less used, and frowned upon because of other's assumed (lack of) expertise, never based on one's own. Shouldn't your junior colleagues be encouraged to learn a variety of language constructs? (like you, presumably, have already done) – haelix May 11 '14 at 10:51

3 Answers3

12

The second case:

return result = Result::INSUCCESS, nullptr;

looks like a gcc bug, it appears to be ignoring the left hand side of the comma operator in the context of a return statement, if I change the return statement to this (see it live):

return (printf("hello\n"), nullptr);

there is no output but if we do this outside of a return statement we get the expected result. It appears to be fixed in 4.8 but I can reproduce with 4.6 and 4.7.

If we look at the draft C++ standard section 5.18 Comma operator it says (emphasis mine):

A pair of expressions separated by a comma is evaluated left-to-right; the left expression is a discarded value expression (Clause 5).83 Every value computation and side effect associated with the left expression is sequenced before every value computation and side effect associated with the right expression. The type and value of the result are the type and value of the right operand; the result is of the same value category as its right operand, and is a bit-field if its right operand is a glvalue and a bit-field. If the value of the right operand is a temporary (12.2), the result is that temporary.

Regardless, as several people have already mentioned this code is clever but hard to read and therefore will be hard to maintain, splitting this into two lines will work fine. I always like to keep in mind this quote from Brian Kernighan (there may be a few other versions of this):

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?

For the first case, the error is valid, if we look at section 4.10 Pointer conversions which says (emphasis mine going forward):

A null pointer constant is an integral constant expression (5.19) prvalue of integer type that evaluates to zero or a prvalue of type std::nullptr_t. A null pointer constant can be converted to a pointer type; the result is the null pointer value of that type and is distinguishable from every other value of object pointer or function pointer type. Such a conversion is called a null pointer conversion.

the expression:

result = Result::INSUCCESS, NULL

is not a constant expression since it contains =, what is a constant expression is covered in section 5.19 Constant expressions and says:

A conditional-expression is a core constant expression unless it involves one of the following as a potentially evaluated subexpression (3.2) [...]

and includes:

an assignment or a compound assignment (5.17); or

Using nullptr is okay since it is a prvalue of std::nullptr_t, we can see that from section 12.14.7 Pointer literals which says:

The pointer literal is the keyword nullptr. It is a prvalue of type std::nullptr_t. [ Note: std::nullptr_t is a distinct type that is neither a pointer type nor a pointer to member type; rather, a prvalue of this type is a null pointer constant and can be converted to a null pointer value or null member pointer value. See 4.10 and4.11. —endnote]

Shafik Yaghmour
  • 154,301
  • 39
  • 440
  • 740
  • That `return result = Result::INSUCCESS, NULL;` fails to compile is compliant as far as I can tell. `#define NULL 0`; then `a = b, 0` is not a constant expression, therefore no null pointer constant, therefore not convertible to `void*`. – dyp May 09 '14 at 15:29
  • @dyp I clarified I was only covering the second case in my answer. – Shafik Yaghmour May 09 '14 at 15:31
  • I don't understand, why `a = b, 0` is not a null-pointer constant, when `0` is. Otherwise put, why `a = b, 0` cannot go where `0` goes. – haelix May 09 '14 at 16:19
  • @haelix it is not a core constant expression because it contains `=` which is covered in `5.19` *Constant expressions*. I will update the answer later on with more details. – Shafik Yaghmour May 09 '14 at 16:29
0

My test indicates that your code should work. I ran this on compileonline.com:

#include <iostream>

using namespace std;

int func5() {
    return 5;
}

int func(int& result) {
    return result = func5(), 10;
}

enum class Result : uint32_t {SUCCESS = 0, INSUCCESS = 1};

void* func(Result& result)
{
    // works great
    /*
    result = Result::INSUCCESS;
    return NULL;
    */

    // error: invalid conversion from ‘long int’ to ‘void*’ [-fpermissive]
    /*
    return result = Result::INSUCCESS, NULL;
    */

    // compiles, but <result> is not set???
    return result = Result::INSUCCESS, nullptr;
}

int main()
{
    int b = 0;
    int a = func(b);
    cout << a << ", " << b << endl;

    Result result = Result::SUCCESS;
    func(result);
    cout << uint32_t(result) << endl;

}

Result:

Compiling the source code....
$g++ -std=c++11 main.cpp -o demo -lm -pthread -lgmpxx -lgmp -lreadline 2>&1

Executing the program....
$demo 
10, 5
1

Perhaps you have a bug in your compiler?

Richard Hodges
  • 68,278
  • 7
  • 90
  • 142
-2

Quote from the C++ standard - comma operator:

The type and value of the result are the type and value of the right operand

so You get 'nullptr', which is converted to the result, also SUCCESS.

Shafik Yaghmour
  • 154,301
  • 39
  • 440
  • 740
cpp-progger
  • 406
  • 3
  • 6