Skip to content

Commit

Permalink
Roll back 9.6.1 RhsHandler and ClientMessageHandler change
Browse files Browse the repository at this point in the history 
Ultimately it's not possible to simply use C-types in SML and get a portable
lib out of it, since C++ classes are used extensively. Creating a C-wrapper for
libSoar would be a large project of its own, and using the C++ code as-is is not
really viable at this point; we would need a SWIG-like tool (why on earth does
SWIG not support C++ to C wrapping?).

Fix and remove outstanding TODOs from the whole refactoring. Leave the new APIs
in place, as they are more ergonomic C++, anyway. Remove all logic related to
the buffer resizing, and remove duplicate code where possible.

Add documentation to the new methods.
  • Loading branch information
@garfieldnate
garfieldnate committed Aug 31, 2023
1 parent 06a2885 commit 45055b7
Show file tree
Hide file tree
Showing 15 changed files with 91 additions and 233 deletions.
39 changes: 5 additions & 34 deletions Core/ClientSML/src/sml_ClientEvents.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,50 +69,21 @@ namespace sml
// You need to delete ClientXML objects you create and you should not delete the pXML object you are passed.
typedef void (*XMLEventHandler)(smlXMLEventId id, void* pUserData, Agent* pAgent, ClientXML* pXML) ;


// TODO: move to Kernel::AddRhsHandler, which clients actually see. Expand on why this is the way it is.
// Maintainer note: RhsEventHandler and ClientMessageHandler require special handling:
// RETURN: returns pointer to buff. If buff is not large enough, it will return NULL and set buffSize to a new size. Reallocate the new size and call again.
// Implementations should save a static value locally to be returned when the client retries the call. This also means that the client
// MUST re-do the call if NULL was returned, or else the next call will return an unrelated value.
// Recommended implementation:
// // at beginning of function:
// static std::string prevResult;
// if ( !prevResult.empty() )
// {
// strncpy( buf, prevResult.c_str(), *bufSize );
// prevResult = "";
// return buf;
// }
// ...
// // at end of function
// if ( resultString.length() + 1 > *bufSize )
// {
// *bufSize = resultString.length() + 1;
// prevResult = resultString;
// return NULL;
// }
// strcpy( buf, resultString.c_str() );
// return buf;

// Handler for RHS (right hand side) function firings
// pFunctionName and pArgument define the RHS function being called (the client may parse pArgument to extract other values)
// The return value is a string which allows the RHS function to create a symbol: e.g. ^att (exec plus 2 2) producing ^att 4
// SEE MAINTAINER NOTE ABOVE!
typedef char const *(*RhsEventHandler)(smlRhsEventId id, void* pUserData, Agent* pAgent,
char const* pFunctionName, char const* pArgument, int *buffSize, char *buff) ;
typedef std::string (*RhsEventHandler)(smlRhsEventId id, void* pUserData, Agent* pAgent,
char const* pFunctionName, char const* pArgument) ;

using RhsEventHandlerCpp = std::function<std::string(smlRhsEventId id, Agent *pAgent, char const *pFunctionName, char const *pArgument)>;

// Handler for a generic "client message". The content is determined by the client sending this data.
// The message is sent as a simple string and the response is also a string. The string can contain data that is intended to be parsed,
// such as a simple series of integers up to a complete XML message.
// SEE MAINTAINER NOTE ABOVE!
// TODO: add alternative similar to RhsEventHandlerCpp above
typedef char const *(*ClientMessageHandler)(smlRhsEventId id, void *pUserData, Agent *pAgent,
char const *pFunctionName, char const *pArgument, int *buffSize, char *buff);
typedef std::string(*ClientMessageHandler)(smlRhsEventId id, void *pUserData, Agent *pAgent,
char const *pClientName, char const *pArgument);

using ClientMessageHandlerCpp = std::function<std::string(smlRhsEventId id, Agent *pAgent, char const *pFunctionName, char const *pArgument)>;
using ClientMessageHandlerCpp = std::function<std::string(smlRhsEventId id, Agent *pAgent, char const *pClientName, char const *pArgument)>;

// We'll store a handler function together with a generic pointer to data of the user's choosing
// (which is then passed back into the handler when the event occurs).
Expand Down
55 changes: 24 additions & 31 deletions Core/ClientSML/src/sml_ClientKernel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -671,8 +671,6 @@ void Kernel::ReceivedRhsEvent(smlRhsEventId id, AnalyzeXML* pIncoming, ElementXM
return ;
}

// TODO: since we're only using the name, we could get rid of the handler+data wrapper class
// and iterate over name/handler key/vals here instead of looking up the name in the wrapper
// Look up the handler(s) from the map
RhsEventMap::ValueList* pHandlers = m_RhsEventMap.getList(pFunctionName) ;

Expand Down Expand Up @@ -2078,7 +2076,7 @@ int Kernel::InternalAddRhsFunction(smlRhsEventId id, char const* pRhsFunctionNam
bool found = m_RhsEventMap.findFirstValueByTest(&test, &optionalFoundHandler) ;
if (found && optionalFoundHandler.m_Handler != 0)
{
// TODO: log a warning here to help catch copy/paste errors in the client
std::cerr << "WARNING: Attempt to register a duplicate RHS function handler for " << pRhsFunctionName << std::endl ;
return optionalFoundHandler.getCallbackID() ;
}

Expand Down Expand Up @@ -2134,37 +2132,21 @@ bool Kernel::InternalRemoveRhsFunction(smlRhsEventId id, int callbackID)
return true ;
}

// TODO: document
RhsEventHandlerCpp c2cppHandler(RhsEventHandler handler, void* pUserData) {
// RHS function and client message handlers can be added via 2 separate API's; this helper converts
// the function pointer-style handlers into std::function-style handlers, because the
// std::function-style is used internally.
RhsEventHandlerCpp funPointer2StdFunction(RhsEventHandler handler, void* pUserData) {
return [handler, pUserData](
smlRhsEventId id,
Agent* pAgent,
char const* pFunctionName,
char const* pArgument) -> std::string {
char buffer[SML_INITIAL_BUFFER_SIZE] = {0};
char *buff = buffer;
std::string result;
int buffSize = sizeof(buffer);

const char *retVal = handler(id, pUserData, pAgent, pFunctionName, pArgument, &buffSize, buff) ;

if ( !retVal )
{
buff = new char[buffSize];

result = handler(id, pUserData, pAgent, pFunctionName, pArgument, &buffSize, buff) ;
// TODO: check for NIL again and print error of some kind
delete [] buff;
}
else {
result = retVal;
};
return result;
return handler(id, pUserData, pAgent, pFunctionName, pArgument) ;
};
}

/*************************************************************
* @brief Register a handler for a RHS (right hand side) function.
* @brief Register a handler for an RHS (right hand side) function.
* This function can be called in the RHS of a production firing
* allowing a user to quickly extend Soar with custom methods added to the client.
*
Expand All @@ -2189,10 +2171,18 @@ RhsEventHandlerCpp c2cppHandler(RhsEventHandler handler, void* pUserData) {
*************************************************************/
int Kernel::AddRhsFunction(char const* pRhsFunctionName, RhsEventHandler handler, void* pUserData, bool addToBack)
{
RhsEventHandlerCpp newHandler = c2cppHandler(handler, pUserData);
RhsEventHandlerCpp newHandler = funPointer2StdFunction(handler, pUserData);
return AddRhsFunction(pRhsFunctionName, newHandler, addToBack) ;
}


/*****************************************************
* @brief Register a handler for an RHS (right hand side) function. This is functionally the
* same as the method with the same name that takes a function pointer, but it uses
* std::function instead, which is more ergonomic C++.
*
* @see sml::Kernel::AddRhsFunction(char const* pClientName, RhsEventHandler handler, void* pUserData, bool addToBack)
*/
int Kernel::AddRhsFunction(char const* pRhsFunctionName, RhsEventHandlerCpp handler, bool addToBack)
{
smlRhsEventId id = smlEVENT_RHS_USER_FUNCTION ;
Expand All @@ -2217,9 +2207,6 @@ bool Kernel::RemoveRhsFunction(int callbackID)
* When the original client sends a message, the RHS function handler is called to process and (optionally) return
* a message to the caller.
*
* Multiple handlers can be registered for a given message type and the results will be concatenated together and returned
* to the original caller. (This is expected to be an usual situation).
*
* A RHS (right hand side) function handler is used just to reduce the number of types in the system and because it is sufficient
* for this purpose.
*
Expand All @@ -2236,11 +2223,17 @@ bool Kernel::RemoveRhsFunction(int callbackID)
*************************************************************/
int Kernel::RegisterForClientMessageEvent(char const* pClientName, ClientMessageHandler handler, void* pUserData, bool addToBack)
{
RhsEventHandlerCpp newHandler = c2cppHandler(handler, pUserData);
RhsEventHandlerCpp newHandler = funPointer2StdFunction(handler, pUserData);
return RegisterForClientMessageEvent(pClientName, newHandler, addToBack) ;
}

// TODO: document
/*****************************************************
* @brief Register a handler for receiving generic messages sent from another client. This is
* functionally the same as the method with the same name that takes a function pointer, but it
* uses std::function instead, which is more ergonomic C++.
*
* @see sml::Kernel::RegisterForClientMessageEvent(char const* pClientName, ClientMessageHandler handler, void* pUserData, bool addToBack)
*/
int Kernel::RegisterForClientMessageEvent(char const* pClientName, ClientMessageHandlerCpp handler, bool addToBack) {
smlRhsEventId id = smlEVENT_CLIENT_MESSAGE ;
// We actually use the RHS function code internally to process this message (since it's almost exactly like calling a RHS function that's
Expand Down
19 changes: 15 additions & 4 deletions Core/ClientSML/src/sml_ClientKernel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
#include "sml_ClientEvents.h"
#include "sml_ListMap.h"

#define SML_INITIAL_BUFFER_SIZE 1024

// Forward declare so clients can use this
struct ElementXML_InterfaceStruct;
typedef ElementXML_InterfaceStruct* ElementXML_Handle ;
Expand Down Expand Up @@ -781,7 +779,14 @@ namespace sml
* @returns Unique ID for this callback. Required when unregistering this callback.
*************************************************************/
int AddRhsFunction(char const* pRhsFunctionName, RhsEventHandler handler, void* pUserData, bool addToBack = true) ;
// TODO: document

/*****************************************************
* @brief Register a handler for an RHS (right hand side) function. This is functionally the
* same as the method with the same name that takes a function pointer, but it uses
* std::function instead, which is more ergonomic C++.
*
* @see sml::Kernel::AddRhsFunction(char const* pClientName, RhsEventHandler handler, void* pUserData, bool addToBack)
*/
int AddRhsFunction(char const* pRhsFunctionName, RhsEventHandlerCpp handler, bool addToBack = true) ;

/*************************************************************
Expand Down Expand Up @@ -818,7 +823,13 @@ namespace sml
* @returns Unique ID for this callback. Required when unregistering this callback.
*************************************************************/
int RegisterForClientMessageEvent(char const* pClientName, ClientMessageHandler handler, void* pUserData, bool addToBack = true) ;
// TODO: document
/*****************************************************
* @brief Register a handler for receiving generic messages sent from another client. This is
* functionally the same as the method with the same name that takes a function pointer, but it
* uses std::function instead, which is more ergonomic C++.
*
* @see sml::Kernel::RegisterForClientMessageEvent(char const* pClientName, ClientMessageHandler handler, void* pUserData, bool addToBack)
*/
int RegisterForClientMessageEvent(char const* pClientName, ClientMessageHandlerCpp handler, bool addToBack = true) ;

/*************************************************************
Expand Down
14 changes: 3 additions & 11 deletions Core/ClientSMLSWIG/CSharp/CSharpCallbackByHand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ typedef char const* (STDCALL* ClientMessageCallback)(int eventID, CallbackDataPt

// This is the C++ handler which will be called by clientSML when the event fires.
// Then from here we need to call back to C# to pass back the message.
static const std::string RhsEventHandler(sml::smlRhsEventId /*id*/, CallbackDataPtr pUserData, sml::Agent* pAgent, char const* pFunctionName, char const* pArgument)
static std::string RhsEventHandler(sml::smlRhsEventId /*id*/, CallbackDataPtr pUserData, sml::Agent* pAgent, char const* pFunctionName, char const* pArgument)
{
// The user data is the class we declared above, where we store the Java data to use in the callback.
CSharpCallbackData* pData = (CSharpCallbackData*)pUserData ;
Expand All @@ -834,14 +834,6 @@ static const std::string RhsEventHandler(sml::smlRhsEventId /*id*/, CallbackData
return res;
}

static const sml::RhsEventHandlerCpp getRhsEventHandler(void *pUserData)
{
return [pUserData](sml::smlRhsEventId id, sml::Agent *pAgent, char const *pFunctionName, char const *pArgument) -> const std::string
{
return RhsEventHandler(id, pUserData, pAgent, pFunctionName, pArgument);
};
}

SWIGEXPORT intptr_t SWIGSTDCALL CSharp_Kernel_AddRhsFunction(kernelPtr jarg1, char const* pFunctionName, kernelPtr jkernel, callbackFnPtr jarg3, CallbackDataPtr jdata)
{
// jarg1 is the C++ Kernel object
Expand All @@ -856,7 +848,7 @@ SWIGEXPORT intptr_t SWIGSTDCALL CSharp_Kernel_AddRhsFunction(kernelPtr jarg1, ch
CSharpCallbackData* pData = CreateCSharpCallbackDataKernel(jkernel, 0, jarg3, jdata) ;

// Register our handler. When this is called we'll call back to the client method.
pData->m_CallbackID = arg1->AddRhsFunction(pFunctionName, getRhsEventHandler(pData)) ;
pData->m_CallbackID = arg1->AddRhsFunction(pFunctionName, &RhsEventHandler, pData) ;

// Pass the callback info back to the client. We need to do this so we can delete this later when the method is unregistered
return reinterpret_cast<intptr_t>(pData) ;
Expand Down Expand Up @@ -899,7 +891,7 @@ SWIGEXPORT intptr_t SWIGSTDCALL CSharp_Kernel_RegisterForClientMessageEvent(kern
CSharpCallbackData* pData = CreateCSharpCallbackDataKernel(jkernel, 0, jarg3, jdata) ;

// Register our handler. When this is called we'll call back to the client method.
pData->m_CallbackID = arg1->RegisterForClientMessageEvent(pClientName, getRhsEventHandler(pData)) ;
pData->m_CallbackID = arg1->RegisterForClientMessageEvent(pClientName, &RhsEventHandler, pData) ;

// Pass the callback info back to the client. We need to do this so we can delete this later when the method is unregistered
return reinterpret_cast<intptr_t>(pData) ;
Expand Down
17 changes: 3 additions & 14 deletions Core/ClientSMLSWIG/Java/JavaCallbackByHand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ static std::string StringEventHandler(sml::smlStringEventId id, void* pUserData,

}

static const std::string handleRhsEvent(sml::smlRhsEventId id, void *pUserData, sml::Agent *pAgent,
static std::string handleRhsEvent(sml::smlRhsEventId id, void *pUserData, sml::Agent *pAgent,
char const *pFunctionName, char const *pArgument)
{
// The user data is the class we declared above, where we store the Java data to use in the callback.
Expand Down Expand Up @@ -273,17 +273,6 @@ static const std::string handleRhsEvent(sml::smlRhsEventId id, void *pUserData,
return resultStr;
}

// This is the C++ handler which will be called by clientSML when the event fires.
// Then from here we need to call back to Java to pass back the message.
static const sml::RhsEventHandlerCpp getRhsEventHandler(void *pUserData)
{
return [pUserData](sml::smlRhsEventId id, sml::Agent *pAgent, char const *pFunctionName, char const *pArgument)
{
return handleRhsEvent(id, pUserData, pAgent, pFunctionName, pArgument);
};
}


#ifdef __cplusplus
// We expose the public methods in a DLL with C naming (not C++ mangled names)
extern "C" {
Expand Down Expand Up @@ -933,7 +922,7 @@ JNIEXPORT jlong JNICALL Java_sml_smlJNI_Kernel_1AddRhsFunction(JNIEnv *jenv, jcl
JavaCallbackData* pJavaData = CreateJavaCallbackData(false, jenv, jcls, jarg1, 0, jarg3, jarg4, "rhsFunctionHandler", "(ILjava/lang/Object;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", jarg6) ;

// Register our handler. When this is called we'll call back to the Java method.
pJavaData->m_CallbackID = arg1->AddRhsFunction(pFunctionName, getRhsEventHandler(pJavaData)) ;
pJavaData->m_CallbackID = arg1->AddRhsFunction(pFunctionName, &handleRhsEvent, pJavaData) ;

// Release the string we got from Java
jenv->ReleaseStringUTFChars(jarg2, pFunctionName);
Expand Down Expand Up @@ -976,7 +965,7 @@ JNIEXPORT jlong JNICALL Java_sml_smlJNI_Kernel_1RegisterForClientMessageEvent(JN
JavaCallbackData* pJavaData = CreateJavaCallbackData(false, jenv, jcls, jarg1, 0, jarg3, jarg4, "clientMessageHandler", "(ILjava/lang/Object;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", jarg6) ;

// Register our handler. When this is called we'll call back to the Java method.
pJavaData->m_CallbackID = arg1->RegisterForClientMessageEvent(pFunctionName, getRhsEventHandler(pJavaData)) ;
pJavaData->m_CallbackID = arg1->RegisterForClientMessageEvent(pFunctionName, &handleRhsEvent, pJavaData) ;

// Release the string we got from Java
jenv->ReleaseStringUTFChars(jarg2, pFunctionName);
Expand Down
Loading

0 comments on commit 45055b7

Please sign in to comment.