diff --git a/common/plugins/altium/altium_parser.cpp b/common/plugins/altium/altium_parser.cpp index 8c5a8a90b2..52e504b301 100644 --- a/common/plugins/altium/altium_parser.cpp +++ b/common/plugins/altium/altium_parser.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include @@ -78,6 +77,16 @@ ALTIUM_PARSER::ALTIUM_PARSER( } +ALTIUM_PARSER::ALTIUM_PARSER( std::unique_ptr& aContent, size_t aSize ) +{ + m_subrecord_end = nullptr; + m_size = aSize; + m_error = false; + m_content = std::move( aContent ); + m_pos = m_content.get(); +} + + std::map ALTIUM_PARSER::ReadProperties() { std::map kv; @@ -89,17 +98,23 @@ std::map ALTIUM_PARSER::ReadProperties() return kv; } + if( length == 0 ) + { + return kv; + } + // There is one case by kliment where Board6 ends with "|NEARDISTANCE=1000mi". // Both the 'l' and the null-byte are missing, which looks like Altium swallowed two bytes. - if( m_pos[length - 1] != '\0' ) + bool hasNullByte = m_pos[length - 1] == '\0'; + if( !hasNullByte ) { wxLogError( "For Altium import, we assumes a null byte at the end of a list of properties. " "Because this is missing, imported data might be malformed or missing." ); } - //we use std::string because std::string can handle NULL-bytes - //wxString would end the string at the first NULL-byte - std::string str = std::string( m_pos, length - 1 ); + // we use std::string because std::string can handle NULL-bytes + // wxString would end the string at the first NULL-byte + std::string str = std::string( m_pos, length - ( hasNullByte ? 1 : 0 ) ); m_pos += length; std::size_t token_end = 0; @@ -107,18 +122,28 @@ std::map ALTIUM_PARSER::ReadProperties() { std::size_t token_start = str.find( '|', token_end ); std::size_t token_equal = str.find( '=', token_start ); - token_end = str.find( '|', token_equal ); + token_end = str.find( '|', token_start + 1 ); + + if( token_equal >= token_end ) + { + continue; // this looks like an error: skip the entry. Also matches on std::string::npos + } + + if( token_end == std::string::npos ) + { + token_end = str.size() + 1; // this is the correct offset + } std::string keyS = str.substr( token_start + 1, token_equal - token_start - 1 ); std::string valueS = str.substr( token_equal + 1, token_end - token_equal - 1 ); - //convert the strings to wxStrings, since we use them everywhere - //value can have non-ASCII characters, so we convert them from LATIN1/ISO8859-1 + // convert the strings to wxStrings, since we use them everywhere + // value can have non-ASCII characters, so we convert them from LATIN1/ISO8859-1 wxString key( keyS.c_str(), wxConvISO8859_1 ); wxString value( valueS.c_str(), wxConvISO8859_1 ); // Altium stores keys either in Upper, or in CamelCase. Lets unify it. - kv.insert( { key.Trim().MakeUpper(), value.Trim() } ); + kv.insert( { key.Trim( false ).Trim( true ).MakeUpper(), value.Trim() } ); } return kv; diff --git a/common/plugins/altium/altium_parser.h b/common/plugins/altium/altium_parser.h index 0588309413..891b16f878 100644 --- a/common/plugins/altium/altium_parser.h +++ b/common/plugins/altium/altium_parser.h @@ -47,6 +47,7 @@ class ALTIUM_PARSER { public: ALTIUM_PARSER( const CFB::CompoundFileReader& aReader, const CFB::COMPOUND_FILE_ENTRY* aEntry ); + ALTIUM_PARSER( std::unique_ptr& aContent, size_t aSize ); ~ALTIUM_PARSER() = default; template diff --git a/qa/common/plugins/altium/test_altium_parser.cpp b/qa/common/plugins/altium/test_altium_parser.cpp index 11f8bd3652..163a6dcac5 100644 --- a/qa/common/plugins/altium/test_altium_parser.cpp +++ b/qa/common/plugins/altium/test_altium_parser.cpp @@ -215,4 +215,89 @@ BOOST_AUTO_TEST_CASE( PropertiesReadKicadUnit ) } } +struct READ_PROPERTIES_CASE +{ + std::string input; + std::map exp_result; +}; + +/** + * A list of valid test strings and the expected result map + */ +static const std::vector read_properties = { + // Empty + { "", {} }, + { "\0", {} }, + { "|", {} }, + { "|\0", {} }, + { "||", {} }, + { "||\0", {} }, + // Empty key-value pair + { "|=", { { "", "" } } }, + { "|=\0", { { "", "" } } }, + { "| = ", { { "", "" } } }, + { "| = \0", { { "", "" } } }, + // Single key-value pair + { "|A=\0", { { "A", "" } } }, + { "|A=B", { { "A", "B" } } }, + { "|A=B\0", { { "A", "B" } } }, + { "|A=B|", { { "A", "B" } } }, + { "|A=B|\0", { { "A", "B" } } }, + // Multiple key-value pairs + { "|A=B|C=D|\0", { { "A", "B" }, { "C", "D" } } }, + // Same key multiple times + { "|A=B|A=C\0", { { "A", "B" } } }, + { "|A=B|A=C|A=D|A=E|A=F\0", { { "A", "B" } } }, + // Always upper case key + { "|a=b\0", { { "A", "b" } } }, + { "|abc123=b\0", { { "ABC123", "b" } } }, + // Trim whitespaces, TODO: correct? + { "|A= B\0", { { "A", " B" } } }, + { "|A=B \0", { { "A", "B" } } }, + { "| A=B\0", { { "A", "B" } } }, + { "|A =B\0", { { "A", "B" } } }, + { "|A=\nB\n\0", { { "A", "\nB" } } }, + // Escaping and other special cases, TODO: extend + //{ "|A=||\0", {{"A", "|"}} }, + { "|A==\0", { { "A", "=" } } }, + { "|A=a\na\0", { { "A", "a\na" } } }, + { "|A=a\ta\0", { { "A", "a\ta" } } }, + // Encoding, TODO: extend + // Correct reading errors + { "|A|B=C\0", { { "B", "C" } } }, + { "|A=B|C\0", { { "A", "B" } } }, +}; + +/** + * Test conversation from binary to properties + */ +BOOST_AUTO_TEST_CASE( ReadProperties ) +{ + for( const auto& c : read_properties ) + { + BOOST_TEST_CONTEXT( wxString::Format( wxT( "'%s'" ), c.input ) ) + { + size_t size = 4 + c.input.size(); + std::unique_ptr content = std::make_unique( size ); + + *content.get() = c.input.size(); + std::memcpy( content.get() + 4, c.input.c_str(), c.input.size() ); + + ALTIUM_PARSER parser( content, size ); + + std::map result = parser.ReadProperties(); + + BOOST_CHECK_EQUAL( parser.HasParsingError(), false ); + BOOST_CHECK_EQUAL( parser.GetRemainingBytes(), 0 ); + + BOOST_CHECK_EQUAL( result.size(), c.exp_result.size() ); + for( const auto& kv : c.exp_result ) + { + BOOST_CHECK_EQUAL( 1, result.count( kv.first ) ); + BOOST_CHECK_EQUAL( result.at( kv.first ), kv.second ); + } + } + } +} + BOOST_AUTO_TEST_SUITE_END()