I re-assign the bug to Petr since he was faster
at least this time :=)

You can get rid of typedef which are unused
and not part of external HLA interface
which should be the case for the type
defined inside certi:: namespace like those.

Applications should not use certi:: types,
the Billard does (certi::MessageBuffer)
but the HLATestsSuite and HLA_Tutorial do not.

I suggest not to reuse those type unless it's appropriate.
For example, I think:

typedef char *AttributeValue

should be deleted and replaced with

typedef std::string AttributeValue_t

I'd like to use _t suffix in order
to make it clear it's a type.

If you encounter type which are still in use
(like typedef char *ObjectName ; )
but may be replace with std::string do not
change them with this patch.

We should do it separatly in order not to include
two much changes in a single patch, this would
make it harder to track regression (if any).

Petr,

If you don't have time to update your patch,
apply-it and I will do the suggested update myself.

The suggested change have no functionnal impact so your
patch is OK.

Hi Petr,

Fast and nice fix thanks,
I propose you some pruning.

My modification proposal would be:

1) Use meaningfull type name if possible
typedef std::string AttributeValue_t
typedef std::string ParameterValue_t
such that
vector<ParameterValue_t> will have more meaning
than
vector<std::string> in a method signature

2) Use MessageBuffer::read_string/write_string API
   instead of the read_int64 + read_bytes
   since it already does this (with an int32)
  
   You should update MessageBuffer.cc since
   there was a bug precisely inside the xxxx_string API :=)

3) Since we now have plain vector some of the loop (if not all)
   in NetworkMessage::removeXXXX(USHort Rank)
   may be replaced with STLish equivalent:
         valueArray.erase(Rank);

What do you think?

When discussing bug #23150 we said that std::string is preferred wherever the new/delete char[] causes problems.

I suggest to replace char* in NetworkMessage by std::string. (If you agree, I'll make the change.)

Migrating to std::string wouldn't cause a big penalty in this case because the Values are anyway duplicated everywhere, so the performance is comparable to std::string.

Any update on this? Do you need a helping hand?

I used valgrind and found several strange things in the code:

(1) valgrind reports wrong memory access to a VERY_LOW address number
NetworkMessage.cc:131 and NetworkMessage.cc:138
memcpy(..., (char )(ValueArray[Rank].value), *length)

Couldn't this be caused by a (strange) dereference of ValueArray[Rank].value? It seems to me you're dereferencing char* and casting it back to (char *)

(2) after fixing this, valgrind reported wrong memory access in ObjectClassBroadcastList.cc:111
memcpy(tempValue, buffer, length)

I think buffer is NULL. I think the buffer should have been set by the message->getValue() few lines above, however when buffer is NULL, the getValue RETURNS the newly allocated buffer value. I guess this value is lost here and buffer is still NULL

(3) isn't there too many new char[length]? The buffer is allocated in message->getValue(), then it's allocated before ObjectClassBroadcastList.cc:111 again. What happens to the value allocated in message->getValue()?