Set pointer to element in vector to null, then check whether pointer is null (C++)

Question

I would like to set pointers to some elements in my vector array to NULL (based on a criteria), and then check whether an element pointer is NULL. If the pointer pointing that element is NULL, I remove the element from my vector array.

My compiler is giving me an error, saying that the address expression must be an lvalue or function designator and I do not understand why (line location commented in code). Since I am taking the address of the value using &, am I not seeing if the pointer pointing to that element is NULL?

I included the preceding code as the error may lie there,

Relevant code:

vector<particle> pl = c.particlelist;
vector<particle> noncollision = c.particlelist;
vector<vector<particle>> collisionlist = new vector<vector<particle>>();
for (int i = 0; i < c.numparticles-1; i++){
    particle first = pl[i];
    for (int j = i+1; j < c.numparticles; j++)
    {
        particle second  = pl[j];
        double d = distance(first, second);
        if (d==0)
        {
            vector<particle> temp = {pl[i], pl[j]};
    collisionlist.push_back(temp);
            noncollision[i].setxposint(NULL); 
            noncollision[j].setxposint(NULL);
        }
        else
        {
        }
    }
}
int j = 0;
for (int i = 0; i < noncollision.size(); i++)
{
    if (&(noncollision[i].getxpos()) == NULL) ////// ERROR HERE
    {
        noncollision.erase(noncollision.begin()+i);
    }
    else
    {
        j++;
    }
}

I am new to C++, and if you could suggest a more elegant way to do this, or a fix, it would be much appreciated. I also assume that my method of setting the pointer to an element, noncollision[i].setxposint(NULL); is correct? Can I return an integer using a function, and take the address?

Functions for getxpos and setxposint:

int particle::getxpos(){
return xpos;
}

void particle::setxposint(int b){
xpos = b;
}

Answer 1

You're using & to take a pointer to a temporary vale (the return from getxpos) which isn't allowed; since a temporary will be going away, the address won't be useful in any way so the language doesn't allow it. It certainly wouldn't ever be NULL even if you could get its address.

Answer 2

noncollision[i].setxposint(NULL);

All that line is doing is setting xpos to zero. Generally the term NULL is used with pointers, and 0 is used with things like integers. NULL is usually a macro for 0L anyway.

&(noncollision[i].getxpos()) == NULL

What this is doing, which is incorrect, is attempting to take the address of the return value from the member method getxpos() and compare it to NULL. Whereas what you really want to do is simply see if the function returns zero. So simply change this line to:

noncollision[i].getxpos() == 0

Answer 3

I'll explain why the compiler doesn't understand what you mean.

When you write

&(someFunction())

you are asking for the address of the thing that the function returns. But functions return values. A value doesn't have an address. Variables have addresses.

When something is a word of memory (which will contain a value), it can be used as an lvalue (left-value), because you can put things into that word of memory:

int b = 1; //make room for an `int` on the stack, then put a `1` there.

When something is just a value, it can only ever be used as an rvalue. The following would not compile, for the same reason that your code would not:

int b; //make room for an `int` on the stack.
42 = b; //ERROR, this makes no sense.
if (42 == NULL) { std::cout << "this is never true" << std::endl; }
&42; //ERROR, 42 isn't a piece of memory, it's a value.

(Caveat: you can use values to refer to words in memory: this usage is called a pointer, e.g.

int b = 1;
*((int *)(42)) = b;

meaning "put the value of b into the memory which has the address 42. This compiles fine (but crashes if you're not allowed to write to the memory at 42.)

Answer 4

It seems that you are trying to check pairs of points for collisions. You then record for each point whether it has any collision. This is best handled by a simple list of flags:

std::vector<bool> has_collision(c.numparticles, false); // init: no collisions found

Afterwards:

    if (d==0)
    {
        has_collision[i] = true;
        has_collision[j] = true;
    }

At the end, iterate over the list of flags and get the points that have no collisions:

for (size_t i = 0; i < c.numparticles; ++i)
{
    if (!has_collision[i])
    {
        // whatever
        // possibly push_back pl[i] into some list
    }
}

In addition: using a vector to hold a pair (i,j) of points is confusing. Standard library has the std::pair type for purposes such as this.

Also: you don't need explicit dynamic allocation (new); let Standard Library manage memory for you in a safe, non-confusing way. Instead of

vector<vector<particle>> collisionlist = *new vector<vector<particle>>();

Use

vector<vector<particle>> collisionlist;

(or vector<pair<particle, particle>>, as described above).

Answer 5

It looks to me you're trying to keep track of 'visited' items, not sure exactly in which way.

Instead of "modifying" the items, you could use an "external" mark. A set looks to be fine here. You could use a set of iterators into the particle list, or in this case a set of indices (i,j) which will likely be more stable.

Here's a start:

#include <vector>
#include <set>

struct particle { };

double distance(particle const&, particle const&) { return 1.0; }

struct context
{
    std::size_t numparticles;
    std::vector<particle> particlelist;

    context() : numparticles(100), particlelist(numparticles) {}
};

static context c;

int main()
{
    using std::vector;
    using std::size_t;

    vector<particle> pl = c.particlelist;
    vector<vector<particle>> collisionlist;

    std::set<size_t> collision;

    for(size_t i = 0; i < c.numparticles-1; i++)
    {
        particle first = pl[i];
        for(size_t j = i+1; j < c.numparticles; j++)
        {
            particle second  = pl[j];
            double d = distance(first, second);
            if(d < 0.0001)
            {
                collisionlist.push_back({pl[i], pl[j]});
                collision.insert(i);
                collision.insert(j);
            }
            else
            {
            }
        }
    }

    for(size_t i = 0; i < pl.size(); i++)
    {
        if(collision.end() != collision.find(i))
        {
            // do something
        }
    }

    // alternatively
    for (int index : collision)
    {
        particle& p = pl[index];
        // do something
    }
}

NOTE Be very very wary of floating point comparison like

 if (d==0.0) // uhoh

because it will likely not do what you expect

• How dangerous is it to compare floating point values?
• What is the most effective way for float and double comparison?
• Is floating-point == ever OK?