Whats the standard here and why?

[sid debgupta]

I am attaching this simple code snippet which compiles fine with MS C++ compiler v15 ( which comes with Visual C++ 8 ) but fails with MS C++ compiler v16 ( Visual C++ 10 ) and g++ from MingW.

The long code is below, here is the relevant part and why I think the compiler does not like it.

I have a static member function in class t_threads as so-

static void run()
{
for( set<t_thread>::iterator ix = _s_pobj->_threads.begin(); ix != _s_pobj->_threads.end(); ++ix )
(*ix).run();
}

t_threads manages a set of t_thread objects. In the above function, it starts the threads it manages. The t_thread::run function is non-const ( of course, since it changes the state of the t_thread object ). The iterator "ix" is also non-const. But the compiler promotes the iterator to a const because t_threads::run is a static member function.

The error from g++ is -

w.cpp: In static member function `static void t_threads::run()':
w.cpp:95: error: passing `const t_thread' as `this' argument of `void t_thread::run()' discards qualifiers

This should be allowed, IMO. I need the t_threads::run function to be static because of the way I call it in main(). See below. Of course I could change the code but this is just a small part of a much larger system and I have to change similar constructs in numerous places and I am going to stick with MS C++ compiler v15 for the time being.

The sample code-
==============================================================================

#include <stdlib.h>
#include <assert.h>

#include <iostream>
#include <string>
#include <set>

using namespace std;

// the t_thread object

class t_thread
{
public:

t_thread( int _tid )
: tid_ (_tid), run_(false)
{}
void run() { run_ = true; cout << "run(" << tid_ << ") running!\n"; }

void dump() const { cout << "dump(" << tid_ << ") run=" << run_ << "\n"; }

bool operator<( const t_thread& t2 ) const {
return tid_ < t2.tid_;
}

private:

bool run_;
int tid_;
};

// the t_threads object

class t_threads
{
public:

static int initialize( string& serr )
{
// check for multiple initialization
if( _s_initialized ) {
serr = "t_threads: multiple initialization not allowed!";
return -1;
}

// create the global t_threads object
_s_pobj = new t_threads();

if( !_s_pobj ) {
serr = "t_threads: cannot create new t_threads!";
return -1;
}

// remember initialization
_s_initialized = true;

return 0;
}

static void finalize()
{
delete _s_pobj;
_s_initialized = false;
}

static const t_threads& _this()
{
return *_s_pobj;
}

static void dump()
{
for( set<t_thread>::const_iterator ixc = _s_pobj->_threads.begin(); ixc != _s_pobj->_threads.end(); ++ixc )
(*ixc).dump();
}

static const set<t_thread>& threads()
{
return _s_pobj->_threads;
}

static int create( int _tid )
{
pair<set<t_thread>::iterator,bool> pair_ = _s_pobj->_threads.insert( t_thread( _tid ) );

assert( pair_.second );

return 0;
}

static void run()
{
for( set<t_thread>::iterator ix = _s_pobj->_threads.begin(); ix != _s_pobj->_threads.end(); ++ix )
(*ix).run();
}

private:

t_threads() {}
t_threads( const t_threads& );
t_threads& operator=( const t_threads& );

set<t_thread> _threads;

static t_threads* _s_pobj;
static bool _s_initialized;
};

t_threads* t_threads::_s_pobj = NULL;
bool t_threads::_s_initialized = false;

// main section

int main(int argc, char **argv)
{
string serr;
if( -1 == t_threads::initialize(serr) ) {
cout << "main: t_threads::initialize error " << serr;
return -1;
}

for( int i = 0; i < atoi(argv[1]); i++ )
if( -1 == t_threads::create(i)) {
cout << "main: t_threads::create error " << serr;
return -1;
}

t_threads::dump();
t_threads::run();
t_threads::dump();

t_threads::finalize();

return 0;
}

[kbw]

class t_threads
{
public:
	//...
	static void run()
	{
		for( set<t_thread>::iterator ix = _s_pobj->_threads.begin(); ix != _s_pobj->_threads.end(); ++ix )
			(*ix).run();
	}

private:
	//...
	set<t_thread> _threads;
	static t_threads* _s_pobj;
};

_s_pobj is static. You can't go from a _s_pobj to a non static _threads. Which instance of t_thread should it use, how will it find it? It can't.

All the t_threads members are static. Did you forget to make set<t_thread> _threads static too?

Rather than making all the members of t_threads static, you should consider using non-static methods/data and having a static instance (possibly using a singleton) of t_threads.

[sid debgupta]

Thanks for the reply, but I think you misunderstood the question. The matter is not one of ambiguity ( there is none and it compiles with VC++ 8/CL v15 and works perfectly ).

The question is why g++ and VC++ 10/CL v16 insists on promoting the "ix" iterator
in t_threads::run to a const_iterator, thereby making (*ix) a const t_thread.

[kbw]

I can't see how it can work perfectly the way it is. The code is clearly wrong and doesn't compile on VS2008 contrary to what you say.

[sid debgupta]

The code is not "wrong". It is functional code from a system I developed
and has been running in a production environment for almost 2 years.
You may have made an error in copying the code.

The correct answer ( I got it on bytes and it is quite interesting ).
I quote -
--------------------------------------------------------------------------------------------
In the newer MS C++ and gcc set<>::iterator is defined as a constant iterator and I suspect that is a specific design choice people have made.

However all iterators types are implementation defined so the fact that the definition has changed from on version of MS C++ to another and is different in GCC is not actually breaking the standard at all. It can happen, the standard says so.

I suspect (think I have seen said elsewhere) that the reason for making all iterators constant is that in a set the key is the value. However the set is sorted on the key and if you allowed the key to change that would imply a different sort order. That would mean the code would have to be able to know and alterations to the key and reorder the list when they happened.

That last bit isn't possible so the conclusion is that the key must be constant (actually a fairly normal assumption for a keyed table) however since the key is the value that means the value is constant too and so everyone has change all set iterators to be constant.
---------------------------------------------------------------------------------------------------

[Duthomhas]

Woah, you are an arrogant snot, aren't ya?

You are misunderstanding something. Just because the code worked in the past does not make it right.

[sid debgupta]

I am not arrogant. You are. And you are wrong in what you said in your
original reply. I dont have the time to explain it to you.

[sid debgupta]

Wait a sec. You are someone else, not the person who replied. Doesnt make sense, I was
not talking to you. Just stay out of the conversation if you cannot be polite.

[Duthomhas]

You are the one who is not being polite. You are rude, condescending, and arrogant about your code, and demanding.

I don't care how long your code has worked. It is easy enough to find horrific code that has worked for years more than your (more normal) construct has. It doesn't make it right, and it doesn't make you right to be rude about the advice offered to you and to the people who offer it.

So if your problem is solved, then go away. People with little social skill are often programmers, but most have the social skill to be grateful or at least kind to those attempting to help them. Since your understanding is so vastly superior to ours, why bother with us any more?

Treat others badly and you can fairly expect to be informed of your infractions. That makes sense.

[m4ster r0shi]

kbw wrote:

_s_pobj is static. You can't go from a _s_pobj to a non static _threads. Which instance of t_thread should it use, how will it find it? It can't.

static int initialize( string& serr )
{
// check for multiple initialization
if( _s_initialized ) {
serr = "t_threads: multiple initialization not allowed!";
return -1;
}

// create the global t_threads object
_s_pobj = new t_threads(); //<- This one here

if( !_s_pobj ) {
serr = "t_threads: cannot create new t_threads!";
return -1;
}

// remember initialization
_s_initialized = true;

return 0;
}

kbw wrote:

Rather than making all the members of t_threads static, you should consider using non-static methods/data and having a static instance (possibly using a singleton) of t_threads.

I think that's exactly what he does. Take a look at his constructors, they're all private. The static variable _s_pobj is used to hold the unique instance of the t_threads object he uses.

EDIT: @sid debgupta:
It's good to use code tags when you post code. (use them like this: [code] your code here [/code])
If you don't, don't expect other people to read everything you've posted.

[Albatross]

Sid, acting in all the ways Duoas described doesn't help the situation. Duoas, calling someone an arrogant snot does nothing to help the situation either. Let's all just jump in a lake for a moment.

Sid, Duoas is an experienced programmer with years of experience. He might miss something from time to time as he is only human, but I haven't seen him make any obviously wrong statements yet. I cannot attest for kbw as I haven't searched nearly as many of his posts, but I'd expect something similar.

All experienced programmers here know how to use Ctrl-C or Command-C depending on the platform, and the errors generated by a mess-up with the copy feature are very easy to recognize. That includes kbw and Duoas.

This is a forum; we all have a right to chime in on a conversation if we have something to add. If you want a private text-based conversation, seek out an IRC.

Note that some members of the forum are very easy to irritate by requesting an answer and then saying "nope, not right, you probably made an elementary mistake". Duoas is part of that set, and I don't blame him.

Finally, Duoas is completely right in saying this:

Since your understanding is so vastly superior to ours, why bother with us any more?

If your understanding is vastly superior, then leave. Else, listen to what we have to say. This forum is one of the most experienced and most sought-out, so we who answer might just know what we're talking about, eh?

-Albatross

EDIT: Probably giving the formula suggested below as I sometimes do is a bad idea considering 1095.

[m4ster r0shi]

@Albatross:

You forgot to write "-some formula that generates Albatross' current post count- posts and counting!"

[sid debgupta]

To Albatross - I have been writing code for over 20 years, I am 45 years old. I posted
this topic on bytes and here, and my only interest is finding the answer. Kbw clearly
read the code wrong, maybe he didnt see it closely enough. Thats ok, but he then
came back and said the code did not compile with VC8, which is a verifiably false statement.

In my reply, I wanted to underline that it certainly does compile and it is a snippet
from a system that has been running for a long time and that I wrote about 2 years
ago. It is only when I recently upgraded the target machine to VC10 that I ran into
the compilation error, which I cross-checked with g++.

To this, Doaus jumped in with an unprovoked slur. I could have
slurred back, but I prefer not to fence with kids. I have posted off and on on
various forums on various subjects, probably from when he was in shorts.

It doesnt matter to me, but facts are facts. What is "acted in the various ways "
mean? It is a meaningless statement, a trigger happy one.

No more from me on this.

[Albatross]

Thank you for sharing your... eh... view with us.

I for one dismiss all personal claims that are passed over the internet that cannot be verified. I am not implying you are a liar when you say you've written code for 20 years, but I cannot verify that you have. I also cannot verify that you have lived for 45 years. I also cannot verify what 20 years of programming and 45 years of life have done for you.

I cannot verify that it builds on VC++2008, as I do not have that program, either. I know that it does not compile on g++ 4.2.1, as was almost claimed.

I would like to point out that you did slur back.

You wrote:

I am not arrogant. You are. And you are wrong in what you said in your
original reply. I dont have the time to explain it to you.

What would you call that?

acted in the various ways

Oh, the statement you gave is meaningful, even if it's not the one I used which is even more meaningful due to its formulation which is generally more compliant with the usual templates of the human mind. Much more so than the idiom "trigger happy".

To this, Doaus jumped in with an unprovoked slur. I could have
slurred back, but I prefer not to fence with kids.

Do note that by implying that Duoas is a kid, you have effectively signed your own hate warrant.

Regards,
-Albatross

[kbw]

sid debgupta:

Thats ok, but he then came back and said the code did not compile with VC8, which is a verifiably false statement.

I simply copied your code into a VS2008 project and compiled it. If I say it doesn't compile, it doesn't compile. Why would I say otherwise? The rest was just helpful commentary. But if you don't find it helpful, that's fine; someone may find it so, this is a public forum.

Rather than getting so definsive, why don't you compile what you posted under VS2008?

Also, quoting your experience is largely irrelevant. And if you think it's important, I have more experience than you.

[Galik]

I have to say that I think Sid is getting a bad rap.

He posted a legitimate question and the first answer from kbw was clearly wrong in a few respects as m4ster r0shi pointed out.

Sid then graciously ignored kbw's inaccurate comments and re-iterated his original question.

To which kbw said the code was "clearly wrong".

Sid, then stated that his code was "not wrong" and for everyone's information he posted the correct answer to the problem.

At this point we simply have two people with a difference of opinion about the "wrong" status of the original code.

Where this conversation went over the edge, I believe, is where Duoas made an ill judged observation that I do not believe was particularly fair. And, in fact, I think it was quite rude.

Duoas wrote:

Woah, you are an arrogant snot, aren't ya?

As Albatross said, that didn't help the conversation at all.

But as to whether the original code was "wrong" or not I would say this. It rather depends on what you call "wrong".

Sid is right in that the answer to this question is indeed quite interesting.

Sid wrote:

The correct answer ( I got it on bytes and it is quite interesting ).

I have looked at the C++ standard and although I may have missed something I can not see anywhere that it is stipulated that all iterators to a std::set must be const iterators.

So in this respect it is not "wrong" according to the standard to use iterators that can modify the elements of a std::set.

However doing so is, in my opinion, compromises the abstraction. A std::set is both ordered and unique. So allowing an external reference to modify an individual element exposes the std::set to the danger of having its abstraction corrupted. The reference could be used to modify the set to become unordered or to contain duplicate values.

It seems that library designers have plugged this "flaw" and now all iterators from a std::set are const iterators.

This obviously breaks some code.

Was the previous code "wrong"?

I am sure that could be argued both ways. But it was (as far as I can tell) in compliance with the standard.