CODEWITHSUNDEEP
c++pointersif-statement
fogbit
fogbit

Reputation: 2063

creating and verifying a pointer into "if" statement

Is this code correct?

void foo ( int* p )
{
  if ( int* p2 = p ) // single "="
  {
    *p2++;
  }
}

I always been thinking it's not, but recently i've saw such code in a colleague's of mine sources.

What if "p" is NULL? MS VS 2008 works correct but shows "warning C4706: assignment within conditional expression".

Thank you.

Upvotes: 5

Views: 5369

Answers (8)

sehe
sehe

Reputation: 393134

The warning assignment within conditional expression is usually issued by compilers to prevent cases where you write

 if (a = b) 
 {

where you meant

 if (a == b) // big difference!
 {

In your sample, the 'assignment warning' is actually bogus, since it is, in fact, not an assignment, but rather an intialization:

 {
      int *p2 = p;
      if (p2)
      {

      }
 }

and there is no risk that you'd actually wanted the syntax error (int *p2 == p ?!) instead :)

The rest of your post is perfectly valid C++03 (and later) and just does what it says1.

1 (which in terms of lasting effect, isn't much because

  • p2 is dereferenced without ever doing something with it
  • p2 is incremented without ever doing something with it,

but I guess that's just the sample code? In case it isn't obvious, *p2++ is equivalent to *p2; p2++;)

Upvotes: 8

Gnawme
Gnawme

Reputation: 2399

By raising warning C4706, the compiler is simply questioning whether you actually meant to write if ( int* p2 == p ) instead of if ( int* p2 = p ) as shown.

Per 6.4 of the 2003 C++ Standard, if ( int* p2 = p ) is legal:

The value of a condition [if (condition)] that is an initialized declaration in a statement other than a switch statement is the value of the declared variable implicitly converted to type bool.

If p is NULL, condition (int* p2 = p) fails because the value of p2 is 0, and thus implicitly false, and *p2 is not incremented.

Upvotes: 5

James Kanze
James Kanze

Reputation: 153929

It's formally correct (in C++, but not in C). Allowing a definition as a conditional was added with dynamic_cast, to support such things as:

if ( Derived* p = dynamic_cast<Derived*>( ptrToBase ) ) {
    //  ...
}

The argument was that there would never be a moment where p was in
scope, but not null. After the if, p is no longer in scope.

The argument doesn't hold water, since on one hand, if you add an else, within the else block, p is in scope, and null. And the justification involving limiting the scope really isn't very strong anyway, because if the function is long enough for it to matter, the function is too long and too complex. This construct supports things like:

if ( Derived1* p = dynamic_cast<Derived1*>( ptrToBase ) ) {
    // ...
}
if ( Derived2* p = dynamic_cast<Derived2*>( ptrToBase ) ) {
    // ...
}
// ...

But this really isn't something you want to do in good code.

All in all, I'd avoid the construct, replacing it with something like:

int* p2 = p;
if ( p2 != NULL ) {
    //  ...
}

It's more explicit and more readable. It does mean that p2 is in scope outside the if, but I'd argue that if this causes any problems, the function itself needs refactoring, because it is too long and too complex.

EDIT:

With regards to the warning: it's a complete idiocy on the part of the compiler. There's no assignment within the conditional expression: there's no assignment, period, and there's no “conditional expression” the condition in the original is a declaration, not an expression. If the compiler wanted to warn about something (on stylistic grounds, because it agrees with me that this is bad style), then it should warn about an implicit conversion to bool. (And of course, whether this warning is emitted should be under control of an option. The compiler should compile the language, not impose style, unless one asks it explicitly to warn about specific style issues.)

Upvotes: 4

Eric
Eric

Reputation: 17

I think grammatically the code has no problem. But it is not a good style. If you definitely know there is single "=", you can also wite like this:

if ((int* p2 = p) != NULL)
{
    *p2++;
}

I prefer the pointer compares to NULL.

Upvotes: 0

RetroCoder
RetroCoder

Reputation: 2685

Look at int* p as this "p is a pointer to an int". And, "p2 is also a pointer to an int". You are making p2 point to the pointer that p is also pointing to when you say p2 = p. The statement *p2++; will increment the address by 4 and point to something besides the int you were originally pointing to. Whichi should be a 32bit integer. The pointer increments by 4 on a 32bit compiler. If you use (*p2)++ wrapped in parenthesis it will increment the integer the pointer is pointing to.

Yes it works... wierd.

int ClassPtr::MethodA(int *p)
{
    if (int *p2 = p)
    {
       (*p2)++;
       return *p2;
    }
          return 0; 
}

Upvotes: 0

Neowizard
Neowizard

Reputation: 3017

This code seems odd. It's correct, but I fail to see in which cases is it useful.
It's less readable then simple alternatives (like Oren S. mentioned), and needlessly defines a new variable in order to increment (*p). The parameter p is already a new variable on the stack, why create another one?

Upvotes: 1

jman
jman

Reputation: 11616

If p is NULL, the condition fails. So the code is ok.

Upvotes: 1

OSH
OSH

Reputation: 2937

the code is error prone (why not break it into 2 statements) but otherwise correct. if you break this into:

void foo ( int* p )
{
  int* p2 = p;
  if ( p2 ) 
  {
    *p2++;
  }
}

it is more readable and won't give you the warning. You can see it's also correct (in case of null you are just assigning p2 with null, which is ok).

Upvotes: 2

Related Questions