Re: Critique ThreadQueue class please

Jim Langston wrote:
"Ulrich Eckhardt" <doomster@xxxxxxxx> wrote in message
news:45s0i4F88iekU1@xxxxxxxxxxxxxxxx
Jim Langston wrote:
The copy constructor and assignment operator have been declared private
cause they just don't work.

The way you did it is wrong. What you do is declare(!) them private and
_not_ implement them in any way. This means that nobody outside the
class can access them and if anything inside by accident uses them, they
at least get a linker error. This should be a common technique known to
any C++ programmer.

Yeah, I know this. I had tried to implement a copy constructor and
assignment operator but couldn't get them to work. I left the code
intact in case I ever want to try again, but moved them to private
so no one could call them. If I can't figure out the way to implement
them I'll delete the code. I'd like to be able to implement them so I
don't have to call new on the ThreadQueue class.

There is no reason to make things copyable and assignable. In particular
classes that are entities instead of mere values are usually not copyable,
prime example being a std::fstream - there are simply no sensible copying
semantics that people would find intuitive and could agree on, so those
objects aren't copyable.

template<typename T, typename F > T StrmConvert( F from )
{
std::stringstream temp;
temp << from;
T to = T();
temp >> to;
return to;
}

#include <boost/lexical_cast.hpp>

That one also checks that the conversion was successful.

The dynamic_cast that I'm going to be changing to static_cast per your
request? Is it really needed?

No, take a look at Boost's lexical_cast function. It pretty much does the
same as your code above just that it provides sound error checking.


ThreadQueue(unsigned int limit): Shutdown( false ), ThreadID( 0 )
{
Limit = limit; // Only saved for copy and assignment
constructors which aren't used cause can't figure out good way

Okay, since assignment op and cctor can be dropped anyway, the comment
that you should use initialisation instead of assignment is also moot.

What comment?

The comment which I didn't make that you should use initialisation instead
of assignment. And I didn't make it because the offending line could be
removed anyway.
;)

handles[SemaphoreIndex] = ::CreateSemaphore(
NULL, // no security attributes
0, // initial count
limit, // max count
NULL); // anonymous

::InitializeCriticalSection(&lock);
}

How do you handle failures in the ctor? Throw a std::bad_alloc!

Unfortunately, I don't handle failures right now. I wasn't really aware,
however, how the ctor could fail. The only way I could think of is if I
run out of memory (which I do if I try to make 10,000 ThreadQueues in
main. It gets to about 9,850 or so then gives me memory errors on
std::string calls)

Well, CreateSemaphore can fail if there aren't any semaphores left.


bool AddTail(std::string* pmessage)
{
if ( Shutdown )
return false;
bool result;
::EnterCriticalSection(&lock);
MsgQueue.push(pmessage);
result = ::ReleaseSemaphore(handles[SemaphoreIndex], 1, NULL) !=
0;
if (!result)
{
// caller can use ::GetLastError to determine what went wrong
MsgQueue.pop();
}
::LeaveCriticalSection(&lock);
return result;
}

Who owns the string object now? Use std::auto_ptr to make that clear.
Also, this almost automatically makes your function exception-safe.

ThreadQueue itself owns the string object. How would auto_ptr document
this though? There would still be the question about who owned the
object, wouldn't there?

Only a single auto_ptr object can point to the same object at any time. If
that auto_ptr is destroyed, it calls delete on the contained raw pointer.
In particular, this makes clear that

string str;
q.AddTail(&str);

is not valid. First, this will stop compiling because auto_ptr doesn't have
an implicit ctor from a raw pointer but secondly the programmer will
hopefully think about what it means to pass an object to a smart pointer
and realize that it doesn't work with a stack variable.



I personally don't like c++'s exception handling and don't use it as
much as I can. I try to handle errors in code if I'm able. The only
error I've seen this program throw is in std::string when the program
runs out of memory. I'm not sure how to check on memory usage.

I never handled OOM condition in a program, but I do handle the exception
thrown and then abort with a sensible error message.

[unnecessary complicated code]
This code was modified from another class similar that actually [...]
I left it as a multiple object in case[...]

If you come back in two months or even years and you try to figure out why
the code is so complicated you will wish that you had simplified it as
much as possible. Yes, documenting intentions is another option.

protected:

Why protected? What is a derived class supposed to use here that
can't be used by other clients? There's also not even a virtual
dtor or for that matter any virtual function at all.

Normally they would be private if I had designed this class from
scratch, but the class I modified had them as protected so I just
left them. I will make the dtor virtual, however, so that it is
possible to derive from this class.

There is no need for that. If you can't envision a reasonable scenario
where one would derive from it, don't prepare for that. It's the same as
with copying and assignment, not every type lends itself to it.



int result;
if ( ! Interface->AddTail( Message ) )
{
// Should do some error here, but what?
}

Hmmmm, how about removing the returnvalue from AddTail and using an
exception to signal failure? That way, you would have no error checking
code here but all of it concentrated in an enclosing try-catch clause.
Also, why doesn't that one use a timeout?

Isn't a catch block the same as error checking code? I dont' see the
advantage to using try...catch than this.

The advantage is that you can put several statements inside a single
try-catch clause, even recursive calls are easily terminated without
having to check returnvalues at every place. I often only put the
try-catch clause in very central places like around the whole of main().

int main()
{
const static int NumOfQueues = 1000;

main() isn't going to be called multiple times let alone recursively, so
why make this static?

Umm... I just used the suggestion people had about converting defines to
variables. In C code this would of been
#define NumOfQueues 1000
const int NumOfQueues
or even
int NumOfQueues
would both work, I was just trying to document that this is unchanging.
So you suggest I get rid of the static?

Yes, the static makes no sense. Remember, there is file-static (which would
make sense in C because there a constant has external linkage) and
function-static which means the object persists across multiple calls of
the function. C++ then adds class-static, which is the third meaning.

Therefore:
size_t const NumOfQueues = 1000;


Also, the expression '(*x).' is equivalent to 'x->'.

Are you sure about that? For iterators (*x) dereferences the
iterator, but there is no guarantee that an iterator is a pointer.

Right, but every iterator I know overloads both operator * and operator ->.

Another strategy to get the thing with locks right is to split
functions in two nested parts. The enclosing part just locks,
calls the 'real' code and then unlocks again.

Would this mean that I would have about twice as many methods in my
class? The one they call as now which just attempts the lock and the
other that handles the code? In what way does this help?

I forgot to mention that the part that 'does stuff' should be private in
this case. Such a strategy helps establishing pre/postcondition checking
and locking/unlocking in some cases, in particular when not using
exceptions. Otherwise (i.e. with exceptions) it is often better done with
RAII.


Uli

.

Relevant Pages

  • Re: VB6, SQL2000 and Word
    ... Private tempdoc As Word.Document ... use RaiseEvent with the following syntax: ... assignment. ... Private Sub CreateMailMergeDataFileAs String) ...
    (microsoft.public.vb.general.discussion)
  • Re: realtime rl v.2
    ... You have to make a choice between a Rendering Engine knowing how to render objects of different type, or each object knowing how to render itself to a surface. ... If you want to use the second, the surface should be separate from the object and can be used like Object.Render;. ... Start with (Private Properties, Public Properties, Constructors, Private/Protected Methods, Public Methods; for forms add Form Events and Control Events). ... To find it yourself, click Debug->Exceptions and then find the above exception, click the left check box and ok. ...
    (rec.games.roguelike.development)
  • Re: realtime rl v.2
    ... Separate your Objects from their rendering implementation, ... Rendering Engine knowing how to render objects of different type, ... private with the 'private' keyword. ... yourself, click Debug->Exceptions and then find the above exception, click ...
    (rec.games.roguelike.development)
  • Re: GH - Underage Drinking
    ... Mexico of giving alcohol to a minor that is NOT his/her child on their ... Some States allow an exception for consumption when a family member ... Some States allow an exception for consumption on private property. ... Some States also allow exceptions for educational purposes (e.g., ...
    (rec.arts.tv.soaps.abc)
  • Spurious printing: AWTRenderer, FOP
    ... application rendered FOP XML. ... private static final int intEvenAndAll = 0; ... private int intStartNumber; ... catch (Exception exception) ...
    (comp.lang.java.programmer)