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.

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

Feature new speakerfile #19

Merged
merged 16 commits into from
Nov 11, 2021
Merged

Feature new speakerfile #19

merged 16 commits into from
Nov 11, 2021

Conversation

paul-krug
Copy link
Collaborator

Changes the meaning of the 'name' element from the Param and Parameter structs of Glottis.h and Vocaltract.h from 'full meaning of the parameter' to 'Abbreviated parameter'.
Introduces 'description' element from the Param and Parameter structs of Glottis.h and Vocaltract.h that have the meaning 'full meaning of the parameter'.
Removes the 'abbr' element from the Param and Parameter structs of Glottis.h and Vocaltract.h.
Introduces new short names for all static, control and derived parameters in all three glottis models.
Introduces new speakerfile 'JD3.speaker' that has consistent tags across sub-glottal and supra-glottal parameters.
Glottal parameters are now read in by 'name' tag and not by 'index' tag anymore, which is now consistent with the supra-glottal parameters.
Bugs in the corresponding XMLnode helpers were fixed.

I ask for a careful review, as this commit represents a more signifcant modification of the VTL code than usual.

Copy link
Contributor

@Simon-Stone Simon-Stone left a comment

Choose a reason for hiding this comment

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

Please add and run appropriate unit tests to check the new behavior.

Glottis.h Outdated Show resolved Hide resolved
VocalTract.cpp Outdated Show resolved Hide resolved
VocalTract.cpp Outdated Show resolved Hide resolved
VocalTract.h Outdated Show resolved Hide resolved
VocalTractLabApi.sln Show resolved Hide resolved
VocalTractLabApi.sln Show resolved Hide resolved
XmlNode.cpp Outdated
@@ -991,14 +991,15 @@ bool XmlNode::hasAttribute(const string &name)


// ****************************************************************************
/// Returns the integer value of the given attribute, or 0, if the attribute
/// does not exist or is not convertable to integer.
/// Returns the integer value of the given attribute, or an exception, if the attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Exceptions are not returned but thrown. Please revise. Also state the type of exceptions explicitly (std::invalid_argument, std::out_of_range) so that caller can catch it appropriately. We also should differentiate between the std::invalid_argument exception thrown by std::stoi in case of conversion error and the std::invalid_argument. In fact, the optimal solution would be to create a custom exception like so (untested code):

struct XmlException : public std::exception
{
    const char* what() const throw() override
    {
    	return "An XML exception occurred!";
    }
}

struct XmlAttributeNotFound : public XmlException
{
    const char* what() const throw() override
    {
    	return "Could not find attribute!";
    }
}

struct XmlBadAttribute : public XmlException
{
    const char* what() const throw() override
    {
    	return "Attribute could not be converted to expected type";
    }
}

That way we could throw XmlAttributeNotFound instead of std::invalid_argument and catch the std::invalid_argument from std::stoi and rethrow as XmlBadAttribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

std::stoi throw different messages than the other errors. You can already differentiate between them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Identifying an exception by its message is bad practice because it is implementation-dependent. Different compilers may throw different messages. The only safe (and also much more explicit) way is to use custom exceptions here.

XmlNode.cpp Outdated
}


// ****************************************************************************
/// Returns the double value of the given attribute, or 0.0, if the attribute
/// does not exist or is not convertable to double.
/// Returns the double value of the given attribute, or an exception, if the attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as in getAttributeInt()

XmlNode.cpp Outdated Show resolved Hide resolved
XmlNode.cpp Outdated
@@ -1075,7 +1080,7 @@ string XmlNode::getAttributeString(const string &name)
}
else
{
return string("");
throw std::invalid_argument(std::string("Tag '") + name + std::string("' could not be found in node!"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as in getAttributeInt()

@paul-krug
Copy link
Collaborator Author

The unit test already exists, as the API could not even initialize if the new speakerfile was not working correctly.

@Simon-Stone
Copy link
Contributor

The unit test already exists, as the API could not even initialize if the new speakerfile was not working correctly.

Right, agreed.

@paul-krug
Copy link
Collaborator Author

paul-krug commented Nov 11, 2021

Added the custom exceptions.
Tested and working. 'Cord_length' parameters must still be renamed

XmlNode.h Outdated
// Classes for custom XML rlated exceptions.
// ****************************************************************************

class XmlException : public std::exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the XML exceptions to a separate file XmlExceptions.h. The general guideline is to have one class per translation unit, although in this case I think it makes sense to have all the XmlException-derived classes in the same file.

I would generally recommend moving the implementation to a separate CPP file. If you choose not to (since it is very brief), make sure you mark the methods as inline, otherwise we may get compilation errors if the header is included in multiple files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@paul-krug
Copy link
Collaborator Author

Should be ready now

@Simon-Stone Simon-Stone merged commit 366827d into main Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants