Participant parameters

[KonradBreitsprecherBkd]

Get the actual values of SIL Kit parameters that are set by public API and possibly overwritten by the participant configuration.

Instead of a seperate interface per parameter, a enum is handed in specifying which parameter should be obtained:

auto GetParameter(SilKit::Parameter parameter) -> const std::string& override;

Currently, the participantName and registryURI are available:#

enum class Parameter : SilKit_Parameter
{
    //! An undefined parameter
    Undefined = SilKit_Parameter_Undefined,
    //! The name of the participant
    ParticipantName = SilKit_Parameter_ParticipantName,
    //! The registry URI
    RegistryUri = SilKit_Parameter_ReistryUri,
};

Any more parameters?

How to deal with non-string parameters?

[VDanielEdwards]

I would produce a new std::string on each invocation. This API shouldn't be called more than once per parameter (under normal circumstances) anyway. This will also simplify the implementation, because the caching in the Hourglass-API can be dropped.

Suggested change

	virtual auto GetParameter(SilKit::Parameter parameter) -> const std::string& = 0;
	virtual auto GetParameter(SilKit::Parameter parameter) -> std::string = 0;

[VDanielEdwards]

If we return a single const char * (which is owned by the SIL Kit participant and not to be deallocated by the user) then we have to ensure that the string this is pointing to never changes / gets reallocated. This will work for the current set of parameters (participant name and registry uri won't change), but who knows if we want to provide parameters that could change in the future.

I would propose an API that copies the strings to user-provided buffer in a similar manner to snprintf. It must be allowed to specify the data-pointer as nullptr which will only fill in the size (which can then be used to create an appropriately sized buffer).

Same as with the C++ API, I think copying is fine here - these parameters shouldn't be performance critical or queried in hot-loops.

Suggested change

	SilKitAPI SilKit_ReturnCode SilKitCALL SilKit_Participant_GetParameter(const char** outParameterValue,
	SilKit_Parameter parameter,
	SilKit_Participant* participant);
	SilKitAPI SilKit_ReturnCode SilKitCALL SilKit_Participant_GetParameter(char* outParameterValueData,
	size_t *outParameterValueSize,
	SilKit_Parameter parameter,
	SilKit_Participant* participant);

[KonradBreitsprecherBkd]

Ok I see the point now. With my initial proposal, the cached value behind const char** outParameterValue is bound to the participant via the _parameterProvider and will leave a dangling pointer if the participant is deleted.

So the usage would look like:

size_t paramSize;
SilKit_Participant_GetParameter(NULL, &paramSize, SilKit_Parameter_ParticipantName, participant);
char* paramValue = malloc(sizeof(char) * paramSize);
// Hope that the parameter size has not changed in the meantime
SilKit_Participant_GetParameter(paramValue, paramSize, SilKit_Parameter_ParticipantName, participant);
// Print paramValue
free(paramValue)

What happens if the parameter has changed beween the two calls? Do we actually need a two-step process like:

size_t paramSize;
uint64_t parameterRequestID;
// First call already caches the parameterValue (and size) internally and hands over a request ID
SilKit_Participant_RequestParameter(&paramSize, &parameterRequestID, SilKit_Parameter_ParticipantName, participant); 
char* paramValue = malloc(sizeof(char) * paramSize);
// Second call copies the cached value to the user buffer
SilKit_Participant_GetParameter(paramValue, parameterRequestID, participant);
// Print paramValue
free(paramValue)

[VDanielEdwards]

Even if the buffer-pointer isn't nullptr, the 'required' size of the buffer will still be returned through the size-pointer. Therefore the user should check that after the second call (which actually copied the data) the size still matches what was expected before.

Some example code in C++ since it is a little bit less messy than C.

size_t size;
/* check return code */ ... = SilKit_Participant_RequestParameter(nullptr, &size, ...);

std::vector<char> buffer;
while (size != buffer.size())
{
  buffer.resize(size);
  /* check return code */ ... = SilKit_Participant_RequestParameter(buffer.data(), &size, ...);
}

Since buffer.size() starts out as 0, the loop will do zero iterations if the initial size was 0.

If the returned size has changed between the two calls, the buffer will be resized accordingly in the next iteration. This repeats for as long as neccessary.

[VDanielEdwards]

Of course, the function must never actually write more than what is specified through the size-pointer.

[VDanielEdwards]

Alternatively we could not copy in the case that the user-specified buffer size is too short, and return a special error code. I believe this is how the Win32 string-encoding translation functions are designed.

Personally I like the loop approach because it is very simple, and makes it possible to get a prefix of a certain length without jumping through hoops.

[VDanielEdwards]

Even if the buffer-pointer isn't nullptr, the 'required' size of the buffer will still be returned through the size-pointer. Therefore the user should check that after the second call (which actually copied the data) the size still matches what was expected before.

Some example code in C++ since it is a little bit less messy than C.

...

Since buffer.size() starts out as 0, the loop will do zero iterations if the initial size was 0.

If the returned size has changed between the two calls, the buffer will be resized accordingly in the next iteration. This repeats for as long as neccessary.

The example code can be optimized by checking for size > buffer.size() and putting an additional buffer.resize(size) after the loop. This makes 'shrinking' size values more efficient.

[KonradBreitsprecherBkd]

I committed an updated version with the suggested pattern.
The inOutParameterValueSize includes null-termination, as this is what the user has to allocate.
Not sure about the inOut prefix, I think this is the first appearance of a two-way argument in the C API.

SilKit_ReturnCode SilKitCALL SilKit_Participant_GetParameter(char* outParameterValue, size_t* inOutParameterValueSize,
                                                             SilKit_Parameter parameter,
                                                             SilKit_Participant* participant)
try
{
    ASSERT_VALID_OUT_PARAMETER(inOutParameterValueSize);
    ASSERT_VALID_POINTER_PARAMETER(participant);

    auto cppParticipant = reinterpret_cast<SilKit::IParticipant*>(participant);
    auto cppParameter = static_cast<SilKit::Parameter>(parameter);
    auto parameterValue = cppParticipant->GetParameter(cppParameter);

    // outParameterValue == nullptr indicates a size-check only, otherwise copy
    if (outParameterValue != nullptr)
    {
        size_t sizeToCopy;
        if (*inOutParameterValueSize >= parameterValue.size() + 1)
        {
            // Don't copy more than we actually have
            sizeToCopy = parameterValue.size();
        }
        else
        {
            // Don't copy more than the given size
            sizeToCopy = *inOutParameterValueSize - 1;
        }
        parameterValue.copy(outParameterValue, sizeToCopy);
        outParameterValue[sizeToCopy] = '\0';
    }
    *inOutParameterValueSize = parameterValue.size() + 1;
    return SilKit_ReturnCode_SUCCESS;
}
CAPI_CATCH_EXCEPTIONS

Usage in C++:

auto ParameterProvider::GetParameter(SilKit_Participant* participant, Parameter parameter) -> std::string
{
    std::vector<char> buffer;
    size_t size = 0;
    SilKit_Parameter cParameter = static_cast<SilKit_Parameter>(parameter);
    {
        const auto returnCode = SilKit_Participant_GetParameter(nullptr, &size, cParameter, participant);
        ThrowOnError(returnCode);
    }
    while (size > buffer.size())
    {
        buffer.resize(size);
        const auto returnCode = SilKit_Participant_GetParameter(buffer.data(), &size, cParameter, participant);
        ThrowOnError(returnCode);
    }
    buffer.resize(size);

    return std::string{buffer.data()};
}

[VDanielEdwards]

For future reference, this class can be removed and the implementation moved to the participant implementation.

[VDanielEdwards]

Output parameter type

The outParameterValue should be a void *. This enables non-string parameters, e.g., byte blobs.

Trailing nul character

I'm also against both, writing the trailing nul character, and including it in the output length.
If the trailing nul character is required for other APIs, the caller must simply allocate one more byte and can explicitly set that to nul after the GetParameter call.

[KonradBreitsprecherBkd]

Typo