CODEWITHSUNDEEP
c++pointers
MathStudent
MathStudent

Reputation: 310

C++ Pointers -- Weird Loop Behavior

From main, I am trying to call a prime function in mymath.cpp -- it has some very strange behaviour that I do not understand. (Note, the algorithm doesn't work yet -- but that's not what is strange to me.)

The strange thing is, if I comment out the line:

cout << "n:" << lastPrime->pnum <<"\n";

... in mymath.cpp, my loop in main only runs twice. If I leave it in, my loop in main runs all the way up to i = 50;

MAIN.CPP

#include <iostream>
#include <stdlib.h>
#include <time.h>
#include "stat.h"
#include "mymath.h";

using namespace std;

int main()
{
    for (int i = 3; i<= 50; i++)
    {
        if (isPrime(i))
        {
            cout << i << " is prime!\n";
        }
        else
        {
            cout << i << " is NOT prime\n";
        }
    }

    return 0;
}

MYMATH.CPP

#include "mymath.h"
#include <math.h>
#include <iostream>

using namespace std;

prime two;
prime * lastPrime = &two;
prime * firstPrime = &two;

bool isPrime(long long n)
{
  two.pnum=2;
  prime * currentPrime = &two;

  if ( n < 2)
      return false;

  long long squareRoot = sqrt(n);

  while(true)
  {
      if (n % currentPrime->pnum==0)
      {
          //n is divisible by a prime number, nothing left to do.
          return false;
      }
      else
      {
          //n is not divisible by a prime... check next one
          {
          if (currentPrime->pprime == 0 || currentPrime->pnum > squareRoot)
              {
                  //this is prime
                  prime addPrime;
                  addPrime.pnum=n;
                  addPrime.pprime=0;
                  lastPrime->pprime=&addPrime;
                  lastPrime=&addPrime;
                  cout << "n:" << lastPrime->pnum <<"\n";
                  return true;
              }
              else
              {
                  //may not be prime, check next
                  currentPrime = currentPrime->pprime;
              }
          }
      }
  }
  return true;
}

Upvotes: 0

Views: 149

Answers (2)

hmjd
hmjd

Reputation: 122011

The code has undefined behaviour as a local variable, named addPrime, is being used beyond its lifetime:

    lastPrime->pprime=&addPrime;
    lastPrime=&addPrime;
    cout << "n:" << lastPrime->pnum <<"\n";
    return true;
} // 'lastPrime' is now a dangling pointer because it holds the address
  // of 'addPrime' whose lifetime has ended.

To correct, you need to dynamically allocate a prime using new instead. But, it appears (without the definition of prime I am uncertain) the code is building a list of primes encountered. Suggest using a std::vector<prime> to build the list and let it manage memory for you.

If a std::vector<prime> is not an option, for whatever reason, then ensure all instances of prime are dynamically allocated and not a mix of dynamically allocated instances and non-dynamically allocated instances (such as the global two) as it is illegal to delete an object not dynamically allocated.

Upvotes: 3

Pete Becker
Pete Becker

Reputation: 76523

Problems that come and go as you add or remove innocuous code almost always are the result of a bad pointer; sometimes it overwrites something that's important and sometimes it overwrites something that doesn't matter.

In this case, the bad pointer comes from taking the address of addPrime and saving it. At the end of the block addPrime goes away, and the pointer to it becomes invalid.

Upvotes: 2

Related Questions