GeTArrayFromBSON doesn't pass on LogOnErrors (suggestion for improvement)

[btwolfe]

The BaseMessageConverter::GetTArrayFromBSON template definition has its third parameter as the function to run for each element in the array. That definition, i.e.,

const std::function<T(FString, bson_t*, bool&)>& keyToT

does not support passing LogOnErrors parameter to the array element parsing function, resulting in an error message each time GetTArrayFromBSON is called when it's loop hits the end of the array looking for the next element. The ActionlibMsgsGoalStatusArrayConverter::_bson_extract_child_goal_status_array is an example that uses the capture clause of the lambda expression to circumvent this limitation, but since the LogOnErrors parameter is ubiquitous throughout the Converter code, using the capture clause seems like an unnecessarily obscure way to get that variable into the keyToT body. Would it not be cleaner to just add an extra bool parameter to the keyToT definition, i.e., instead of

static TArray<T> GetTArrayFromBSON(FString Key, bson_t* msg, bool &KeyFound, const std::function<T(FString, bson_t*, bool&)>& keyToT, bool LogOnErrors = true)

have

static TArray<T> GetTArrayFromBSON(FString Key, bson_t* msg, bool &KeyFound, const std::function<T(FString, bson_t*, bool&, bool&)>& keyToT, bool LogOnErrors = true)

and then explicitly pass LogOnErrors to the keyToT call in the GetTArrayFromBSON for loop? Granted, that forces a minor change to every instantiation of GetTArrayFromBSON. Food for thought.

[jteuber]

Hello @btwolfe ! I wrote GetTArrayFromBSON a while ago, but that error was the exact reason why I passed false to LogOnErrors in all the predefined array-methods like GetDoubleTArrayFromBSON. I'm not really sure why I still passed the value through to the lambda in ActionlibMsgsGoalStatusArrayConverter::_bson_extract_child_goal_status_array, but I guess it was a debugging thing. So we might as well remove it.

I don't quite see the advantage of passing LogOnErrors through explicitly. If you need that, you can still do that as I did in ActionlibMsgsGoalStatusArrayConverter. In general I guess we mostly would like to supress that error on the past-the-end element. I don't really see how we could do that easily though without supressing all error messages.

[btwolfe]

Why can't GetTArrayFromBSON just use array_size returned by get_array_by_key for the for loop so it doesn't run off the end of the array? So instead of

for (int i = 0; elemFound; ++i) 
{   T temp = keyToT(Key + "." + FString::FromInt(i), msg, elemFound, LogOnErrors);
    if (elemFound)
	ret.Add(temp);
}

have

for (int i = 0; i < array_size; ++i)
{   T temp = keyToT(Key + "." + FString::FromInt(i), msg, elemFound, LogOnErrors);
    ret.Add(temp);
}

[jteuber]

Because

array_size holds the size in byte of the buffer where the returned pointer points to.

(from the documentation of get_array_by_key.) Tbh, I'm not quite sure, what exactly get_array_by_key returns. It doesn't seem to be in any immediately usable format, so I just discarded it like the original implementation did as well.

[btwolfe]

Ah, missed that detail. So the call to get_array_by_key is basically just used to ensure that the array exists and report an error if not. How about this; have the loop call bson_has_field before passing on the call to keyToT, e.g.,

for (int i = 0; elemFound; ++i) 
{   FString iKey = Key + "." + FString::FromInt(i);
    elemFound = bson_has_field(msg, iKey);
    if (elemFound)
    {
        T temp = keyToT(iKey, msg, elemFound, LogOnErrors);
        ret.Add(temp);
    }
}

That will stop the end-of-array log messages but still allow true key errors to display

[jteuber]

Hmmm, that's a good idea. Might be a bit slower, but you shouldn't send big arrays this way anyway. I would be all for that!