Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Throw for bad param in GetParameterValue #465

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Throw for bad param in GetParameterValue #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 57 additions & 53 deletions Core/ClientSML/src/sml_ClientIdentifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,40 +31,40 @@ namespace sml
class StringElement ;
class WorkingMemory ;
class Identifier ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these whitespace changes btw


// Two identifiers (two wmes) can have the same value because this is a graph not a tree
// so we need to represent that separately.
class EXPORT IdentifierSymbol
{
friend class Identifier ; // Provide direct access to children.

protected:
// The value for this id, which is a string identifier (e.g. I3)
// We'll use upper case for Soar IDs and lower case for client IDs
// (sometimes the client has to generate these before they are assigned by the kernel)
std::string m_Symbol ;

// The list of WMEs owned by this identifier.
// (When we delete this identifier we'll delete all these automatically)
std::list<WMElement*> m_Children ;

// The list of WMEs that are using this symbol as their identifier
// (Usually just one value in this list)
std::list<Identifier*> m_UsedBy ;

// This is true if the list of children of this identifier was changed. The client chooses when to clear these flags.
bool m_AreChildrenModified ;

public:
IdentifierSymbol(Identifier* pIdentifier) ;
~IdentifierSymbol() ;

char const* GetIdentifierSymbol()
{
return m_Symbol.c_str() ;
}
void SetIdentifierSymbol(char const* pID);

bool AreChildrenModified()
{
return m_AreChildrenModified ;
Expand All @@ -73,47 +73,47 @@ namespace sml
{
m_AreChildrenModified = state ;
}

// Indicates that an identifier is no longer using this as its value
void NoLongerUsedBy(Identifier* pIdentifier);
void UsedBy(Identifier* pIdentifier);

bool IsFirstUser(Identifier* pIdentifier)
{
if (m_UsedBy.size() == 0)
{
return false ;
}

Identifier* front = m_UsedBy.front() ;
return (front == pIdentifier) ;
}

Identifier* GetFirstUser()
{
return m_UsedBy.front() ;
}

int GetNumberUsing()
{
return (int)m_UsedBy.size() ;
}

// Have this identifier take ownership of this WME. So when the identifier is deleted
// it will delete the WME.
void AddChild(WMElement* pWME) ;
WMElement* GetChildByTimeTag(long long timeTag);
void TransferChildren(IdentifierSymbol* pDestination);
void DeleteAllChildren() ;

void RemoveChild(WMElement* pWME) ;

void DebugString(std::string& result);

private:
std::list<WMElement*>::iterator FindChildByTimeTag(long long timeTag);
} ;

//
// Special note about output-link WMEs: The agent is
// free to remove WMEs from the output-link at any time.
Expand All @@ -134,11 +134,11 @@ namespace sml
friend class IntElement ;
friend class FloatElement ;
friend class OutputDeltaList ; // Allow it to clear are children modified

public:
typedef std::list<WMElement*>::iterator ChildrenIter ;
typedef std::list<WMElement*>::const_iterator ChildrenConstIter ;

ChildrenIter GetChildrenBegin()
{
return m_pSymbol->m_Children.begin() ;
Expand All @@ -147,52 +147,52 @@ namespace sml
{
return m_pSymbol->m_Children.end() ;
}

private:
void ReleaseSymbol();

protected:
// Two identifiers (i.e. two wmes) can share the same identifier value
// So each identifier has a pointer to a symbol object, but two could share the same object.
IdentifierSymbol* m_pSymbol ;

IdentifierSymbol* GetSymbol()
{
return m_pSymbol ;
}
void UpdateSymbol(IdentifierSymbol* pSymbol);

void SetSymbol(IdentifierSymbol* p_ID) ;

void RecordSymbolInMap();

public:
virtual char const* GetValueType() const ;

// Returns a string form of the value stored here.
virtual char const* GetValueAsString() const
{
return m_pSymbol->GetIdentifierSymbol() ;
}

// Returns a string form of the value, uses buf to create the string
virtual char const* GetValueAsString(std::string& buf) const
{
buf.assign(m_pSymbol->GetIdentifierSymbol());
return buf.c_str();
}

// The Identifier class overrides this to return true. (The poor man's RTTI).
virtual bool IsIdentifier() const
{
return true ;
}

virtual Identifier* ConvertToIdentifier()
{
return this;
}

/*************************************************************
* @brief Searches for a child of this identifier that has the given
* time tag.
Expand All @@ -201,7 +201,7 @@ namespace sml
* @param timeTag The tag to look for (e.g. +12 for kernel side or -15 for client side)
*************************************************************/
WMElement* FindFromTimeTag(long long timeTag) const ;

/*************************************************************
* @brief Returns the n-th WME that has the given attribute
* and this identifier as its parent (or NULL).
Expand All @@ -212,7 +212,7 @@ namespace sml
* (> 0 only needed for multi-valued attributes)
*************************************************************/
WMElement* FindByAttribute(char const* pAttribute, int index) const ;

/*************************************************************
* @brief Returns the value (as a string) of the first WME with this attribute.
* (<ID> ^attribute value) - returns "value"
Expand All @@ -222,9 +222,13 @@ namespace sml
char const* GetParameterValue(char const* pAttribute) const
{
WMElement* pWME = FindByAttribute(pAttribute, 0) ;
return pWME ? pWME->GetValueAsString() : NULL ;
if (pWME == NULL){
std::string err_msg = "[Error]: " + this->m_AttributeName + " has no child attribute \"" + std::string(pAttribute) + "\"";
throw std::invalid_argument(err_msg);
}
return pWME->GetValueAsString();
}

/*************************************************************
* @brief Returns the "command name" for a top-level identifier on the output-link.
* That is for output-link O1 (O1 ^move M3) returns "move".
Expand All @@ -233,30 +237,30 @@ namespace sml
{
return this->GetAttribute() ;
}

/*************************************************************
* @brief Adds "^status complete" as a child of this identifier.
*************************************************************/
void AddStatusComplete() ;

/*************************************************************
* @brief Adds "^status error" as a child of this identifier.
*************************************************************/
void AddStatusError() ;

/*************************************************************
* @brief Adds "^error-code <code>" as a child of this identifier.
*************************************************************/
void AddErrorCode(int errorCode) ;

/*************************************************************
* @brief Returns the number of children
*************************************************************/
int GetNumberChildren()
{
return (int)m_pSymbol->m_Children.size() ;
}

/*************************************************************
* @brief Gets the n-th child.
* Ownership of this WME is retained by the agent.
Expand All @@ -265,7 +269,7 @@ namespace sml
* but we want to export this interface to Java/Tcl etc. and this is easier.
*************************************************************/
WMElement* GetChild(int index) ;

/*************************************************************
* @brief This is true if the list of children of this identifier has changed.
* The client chooses when to clear these flags.
Expand All @@ -274,51 +278,51 @@ namespace sml
{
return m_pSymbol->AreChildrenModified() ;
}

StringElement* CreateStringWME(char const* pAttribute, char const* pValue);
IntElement* CreateIntWME(char const* pAttribute, long long value) ;
FloatElement* CreateFloatWME(char const* pAttribute, double value) ;
Identifier* CreateIdWME(char const* pAttribute) ;
Identifier* CreateSharedIdWME(char const* pAttribute, Identifier* pSharedValue) ;

protected:
// This version is only needed at the top of the tree (e.g. the input link)
Identifier(Agent* pAgent, char const* pAttributeName, char const* pIdentifier, long long timeTag);

// The normal case (where there is a parent id)
Identifier(Agent* pAgent, Identifier* pParent, char const* pID, char const* pAttributeName, char const* pIdentifier, long long timeTag) ;
Identifier(Agent* pAgent, IdentifierSymbol* pParentSymbol, char const* pID, char const* pAttributeName, char const* pIdentifier, long long timeTag) ;

// The shared id case (where there is a parent id and value is an identifier that already exists)
Identifier(Agent* pAgent, Identifier* pParent, char const* pID, char const* pAttributeName, Identifier* pLinkedIdentifier, long long timeTag) ;
Identifier(Agent* pAgent, IdentifierSymbol* pParentSymbol, char const* pID, char const* pAttributeName, IdentifierSymbol* pLinkedIdentifierSymbol, long long timeTag) ;

virtual ~Identifier(void);

void AddChild(WMElement* pWME)
{
m_pSymbol->AddChild(pWME) ;
}

void RemoveChild(WMElement* pWME)
{
m_pSymbol->RemoveChild(pWME) ;
}

#ifdef SML_DIRECT
virtual void DirectAdd(Direct_AgentSML_Handle pAgentSML, long long timeTag) ;
#endif

// Send over to the kernel again
virtual void Refresh() ;

private:
// NOT IMPLEMENTED
Identifier(const Identifier& rhs);
Identifier& operator=(const Identifier& rhs);

};

} // namespace

#endif // SML_IDENTIFIER_H
25 changes: 25 additions & 0 deletions Core/ClientSMLSWIG/Python/TestPythonSML.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
# destruction (and maybe some other things, too).
#
# This file needs to be compatible with python 3.5, to run on CI jobs testing the lowest supported python version.
import atexit
from pathlib import Path
import sys
import os
Expand Down Expand Up @@ -90,6 +91,19 @@ def UserMessageCallback(id, tester, agent, clientName, message):
else:
print('✅ Kernel creation succeeded')

# Neglecting to shut down the kernel causes a segfault, so we use atexit to guarantee proper cleanup
# TODO: This should be handled by the Python SML interface automatically.
def __cleanup():
global kernel
try:
if kernel:
kernel.Shutdown()
del kernel
except NameError:
pass

atexit.register(__cleanup)

agent_create_called = CalledSignal()
agentCallbackId0 = kernel.RegisterForAgentEvent(smlEVENT_AFTER_AGENT_CREATED, AgentCreatedCallback, agent_create_called)

Expand Down Expand Up @@ -189,6 +203,17 @@ def test_agent_reinit(agent):

test_agent_reinit(agent)

def test_catch_exception(agent):
input_link = agent.GetInputLink()
try:
input_link.GetParameterValue("non-existent")
except ValueError as e:
print("✅ Correctly caught exception:", e)
else:
assert False, "❌ No exception caught"

test_catch_exception(agent)

def test_agent_destroy(agent):
assert not agent_destroy_called.called, "❌ Agent destroy handler called before destroy"
kernel.DestroyAgent(agent)
Expand Down
7 changes: 7 additions & 0 deletions Core/ClientSMLSWIG/Tcl/TestTclSML.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,13 @@ if { [string first "^rhstest success" [$kernel ExecuteCommandLine "print s1" Soa

set result [$kernel ExecuteCommandLine "init-soar" Soar1]

set input_link [$agent GetInputLink]
if {[catch {$input_link GetParameterValue "non-existent"} msg]} {
puts "✅Correctly caught error:\n$errorInfo\n"
} else {
puts "❌ Catching error FAILED"
}

$kernel DestroyAgent $agent

# remove all the remaining kernel callback functions (not required, just to test)
Expand Down
Loading
Loading