kindred/create
1
1
Fork 0
Code Issues 53 Pull Requests Actions Packages Projects 9 Releases 2 Wiki Activity

Add test causing crash and fix map sorting function. (#22889)

Browse Source 
* Add test causing crash and fix map sorting function.

When opening a model from FreeCAD 0.7, the model would crash when I
changed some of the parameters.  This turned out to be due to the
ElementNameComparator::operator() not correctly sorting the items going
into the map, which caused the map to be invalid.

This change adds a test that represented the exact names causing the
crash as well as a fix for the problem.  Names are now sorted:

1. Empty names first
2. Identifier based names second.
3. Hex based names last.

Identifiers are sorted lexicographically for the name portion and
numerically for the number portion, smallest to largest.

Hex based names are sorted by the value of the hex number, smallest to
largest.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Fix lint issues.

* Add another test form to the mix.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This commit is contained in:
Weston Schmidt
2025-09-05 12:07:02 -07:00
committed by GitHub
parent 6d71a564d5
commit 140c9febc4
2 changed files with 321 additions and 122 deletions
Download Patch File Download Diff File Expand all files Collapse all files

341
src/App/MappedElement.cpp
View File

@@ -31,137 +31,236 @@
#include "DocumentObject.h"
#include "MappedElement.h"
#include <algorithm>
using namespace Data;
// hextoa converts a single hex character to its integer value.
inline
int hextoa(char c)
{
constexpr int HEX_OFFSET = 10;
if (c >= '0' && c <= '9') {
return c - '0';
}
if (c >= 'a' && c <= 'f') {
return c - 'a' + HEX_OFFSET;
}
if (c >= 'A' && c <= 'F') {
return c - 'A' + HEX_OFFSET;
}
return -1; // invalid
}
// getHashLength returns the length of the #hexvalue without the leading '#'.
// If the string does not start with '#', it returns -1.
inline
int getHashLength(const MappedName& name)
{
if (name[0] != '#') {
return 0;
}
int len = name.size();
int i = 1; // start after the '#'
for (; i < len; ++i) {
if (!std::isxdigit(name[i])) {
break;
}
}
return i - 1;
}
// getIntLength returns the length of the integer starting at the given start
// index. If the string does not start with a digit or the start is past the
// end of the string, it returns 0.
inline
int getIntLength(const MappedName& name, int start)
{
int len = name.size();
int i = start;
for (; i < len; ++i) {
if (std::isdigit(name[i]) == 0) {
break; // not a digit, stop counting
}
}
return i - start; // return the length of the integer
}
// compareText compares the text part of two MappedNames starting at the given
// index. It returns -1 if left is less than right, 1 if left is greater than
// right, and 0 if they are equal.
inline
int compareText(const MappedName& left, const MappedName& right, int start)
{
int leftLength = left.size();
int rightLength = right.size();
int minLength = std::min(leftLength, rightLength);
for (int i = start; i < minLength; ++i) {
char ac = left[i];
char bc = right[i];
if (ac != bc) {
return ac < bc ? -1 : 1; // left is less than right
}
}
if (leftLength == rightLength) {
return 0; // they are equal
}
// If we reach here, it means one is a prefix of the other.
return leftLength < rightLength ? -1 : 1; // left is less than right
}
// compareHashed compares two MappedNames that could be hex values, or a
// combination of types. It returns:
// - -1 if left is less than right,
// - 1 if left is greater than right,
// - 0 if they are equal.
// This function handles identifiers being compared against hex values.
inline
int compareHashed(const MappedName& left, const MappedName& right)
{
int leftLength = getHashLength(left);
int rightLength = getHashLength(right);
if (leftLength < 1 || rightLength < 1 || // one of them is not a hex value
leftLength != rightLength) { // they are not the same length
// Choose the shorter one as less favors identifiers first and numbers
// to be smallest to largest.
return leftLength < rightLength ? -1 : 1;
}
// They are the same length and both are hex values, compare the hex digits.
int i = 1;
for (; i <= leftLength; ++i) {
int leftValue = hextoa(left[i]);
int rightValue = hextoa(right[i]);
if (leftValue != rightValue) {
return leftValue < rightValue ? -1 : 1;
}
}
// If we reach here, the hex values are equal, compare the text part.
return compareText(left, right, i);
}
// compareNumbers compares parts of two MappedNames that are identifiers
// containing numbers. It returns:
// - -1 if left is less than right,
// - 1 if left is greater than right,
// - 0 if they are equal.
inline
int compareNumbers(const MappedName& left, const MappedName& right, int start)
{
int leftLength = getIntLength(left, start);
int rightLength = getIntLength(right, start);
if (leftLength != rightLength) {
// The shorter number is less than the longer one.
return leftLength < rightLength ? -1 : 1;
}
int i = start;
for (; i < leftLength; ++i) {
char lc = left[i];
char rc = right[i];
if (lc != rc) {
return lc < rc ? -1 : 1;
}
}
// If we reach here, the numbers are equal, compare the text part.
return compareText(left, right, i);
}
// compareIdentifiers compares two MappedNames that are identifiers.
// It returns:
// - -1 if left is less than right,
// - 1 if left is greater than right,
// - 0 if they are equal.
inline
int compareIdentifiers(const MappedName& left, const MappedName& right)
{
int leftLength = left.size();
int rightLength = right.size();
int minLength = std::min(leftLength, rightLength);
int i = 0;
for (i = 0; i < minLength; ++i) {
char lc = left[i];
char rc = right[i];
bool ldigit = std::isdigit(lc);
bool rdigit = std::isdigit(rc);
if (!ldigit && !rdigit) {
if (lc != rc) {
return lc < rc ? -1 : 1; // left is less than right
}
continue; // equal, continue to next character
}
if (ldigit && rdigit) {
// both identifiers are the same up to this point,
return compareNumbers(left, right, i);
}
return ldigit ? -1 : 1; // left is less than right
}
// The identifier is the entire string, so we compare the lengths.
if (leftLength == rightLength) {
return 0; // they are equal
}
return leftLength < rightLength ? -1 : 1; // left is less than right
}
// Compare and sort the names.
//
// There are two forms of names that we compare:
// - Hex value: #<hex><text> like: #123:8;whatever or #aff-otherstuff;
// - Identifier: <id><number><text> like: Edge123:8;whatever or Edge1234:8;whatever
//
// 1. Empty names are less than any other name.
// 2. Identifiers are ordered before hex values.
// 3. If both names are hex values, compare the hex values, smaller first.
// 3. If both names are identifiers, compare the identifiers, lexicographically
// for the name and numerically for the number, smaller first.
// 6. If the identifiers or hex values are equal, compare the text part lexicographically.
// 7. If the text parts are equal but one is shorter, the shorter name is first.
bool ElementNameComparator::operator()(const MappedName& leftName,
const MappedName& rightName) const
{
int size = static_cast<int>(std::min(leftName.size(), rightName.size()));
if (size == 0U) {
return leftName.size() < rightName.size();
}
int currentIndex = 0;
if (rightName[0] == '#') {
if (leftName[0] != '#') {
return true;
}
// If both string starts with '#', compare the following hex digits by
// its integer value.
int res = 0;
for (currentIndex = 1; currentIndex < size; ++currentIndex) {
auto ac = (unsigned char)leftName[currentIndex];
auto bc = (unsigned char)rightName[currentIndex];
if (std::isxdigit(bc) != 0) {
if (std::isxdigit(ac) == 0) {
return true;
}
if (res == 0) {
if (ac < bc) {
res = -1;
}
else if (ac > bc) {
res = 1;
}
}
}
else if (std::isxdigit(ac) != 0) {
res = 1;
}
else {
break;
}
}
if (res < 0) {
return true;
}
if (res > 0) {
return false;
int leftLength = leftName.size();
int rightLength = rightName.size();
if (std::min(leftLength, rightLength) == 0) {
return leftLength < rightLength; // empty names are less than any other name
}
for (; currentIndex < size; ++currentIndex) {
char ac = leftName[currentIndex];
char bc = rightName[currentIndex];
if (ac < bc) {
return true;
}
if (ac > bc) {
return false;
}
}
return leftName.size() < rightName.size();
}
if (leftName[0] == '#') {
return false;
// If at least one of the names is a hex value, we compare them as such.
if (leftName[0] == '#' || rightName[0] == '#') {
int got = compareHashed(leftName, rightName);
return got < 0; // if leftName is less than rightName, return true
}
// If the string does not start with '#', compare the non-digits prefix
// using lexical order.
for (currentIndex = 0; currentIndex < size; ++currentIndex) {
auto ac = (unsigned char)leftName[currentIndex];
auto bc = (unsigned char)rightName[currentIndex];
if (std::isdigit(bc) == 0) {
if (std::isdigit(ac) != 0) {
return true;
}
if (ac < bc) {
return true;
}
if (ac > bc) {
return false;
}
}
else if (std::isdigit(ac) == 0) {
return false;
}
else {
break;
}
}
int got = compareIdentifiers(leftName, rightName);
// Then compare the following digits part by integer value
int res = 0;
for (; currentIndex < size; ++currentIndex) {
auto ac = (unsigned char)leftName[currentIndex];
auto bc = (unsigned char)rightName[currentIndex];
if (std::isdigit(bc) != 0) {
if (std::isdigit(ac) == 0) {
return true;
}
if (res == 0) {
if (ac < bc) {
res = -1;
}
else if (ac > bc) {
res = 1;
}
}
}
else if (std::isdigit(ac) != 0) {
return false;
}
else {
break;
}
}
if (res < 0) {
return true;
}
if (res > 0) {
return false;
}
// Finally, compare the remaining tail using lexical order
for (; currentIndex < size; ++currentIndex) {
char ac = leftName[currentIndex];
char bc = rightName[currentIndex];
if (ac < bc) {
return true;
}
if (ac > bc) {
return false;
}
}
return leftName.size() < rightName.size();
return got < 0; // if leftName is less than rightName or 0, return true
}
HistoryItem::HistoryItem(App::DocumentObject* obj, const Data::MappedName& name)

104
tests/src/App/MappedElement.cpp
View File

@@ -1,6 +1,11 @@
// SPDX-License-Identifier: LGPL-2.1-or-later
#include <gtest/gtest.h>
#include <utility>
#include <map>
#include <string>
#include <vector>
#include "App/IndexedName.h"
#include "App/MappedElement.h"
@@ -172,13 +177,24 @@ TEST_F(MappedElementTest, comparatorBothStartWithTheSameHexDigits)
EXPECT_FALSE(comp(mappedName1, mappedName2));
}
TEST_F(MappedElementTest, DISABLED_comparatorHexWithoutTerminatorIsBroken)
TEST_F(MappedElementTest, comparatorHexWithoutTerminator)
{
// Arrange
Data::MappedName mappedName1 {"#fed"};
Data::MappedName mappedName2 {"#abcdef"};
auto comp = Data::ElementNameComparator();
// Act & Assert
EXPECT_TRUE(comp(mappedName1, mappedName2));
}
TEST_F(MappedElementTest, comparatorHexWithoutTerminatorSameLength)
{
// Arrange
Data::MappedName mappedName1 {"#fed"};
Data::MappedName mappedName2 {"#abc"};
auto comp = Data::ElementNameComparator();
// Act & Assert
EXPECT_FALSE(comp(mappedName1, mappedName2));
}
@@ -205,7 +221,7 @@ TEST_F(MappedElementTest, comparatorNoHexDigitsSameStringNumericCompare)
EXPECT_FALSE(comp(mappedName1, mappedName2));
}
TEST_F(MappedElementTest, DISABLED_comparatorIntegerWithoutTerminatorIsBroken)
TEST_F(MappedElementTest, comparatorIntegerWithoutTerminatorIsBroken)
{
// Arrange
Data::MappedName mappedName1 {"Edge123456"};
@@ -215,3 +231,87 @@ TEST_F(MappedElementTest, DISABLED_comparatorIntegerWithoutTerminatorIsBroken)
// Act & Assert
EXPECT_FALSE(comp(mappedName1, mappedName2));
}
TEST_F(MappedElementTest, comparatorThreeComplexHexNamesInMap)
{
// Arrange
Data::MappedName name1("#19c9:e;:U;FUS;:Hce4:7,E");
Data::MappedName name2("#1dadb:11;:L#1061a;FUS;:H:d,E");
Data::MappedName name3("#1dae6:8;:L#1dae4;FUS;:H:d,E");
std::map<Data::MappedName, int, Data::ElementNameComparator> testMap;
testMap[name1] = 1;
testMap[name2] = 2;
testMap[name3] = 3;
// Assert: map should have 3 unique keys
EXPECT_EQ(testMap.size(), 3);
// Collect keys in order
std::vector<std::string> keys;
for (const auto& kv : testMap) {
keys.push_back(kv.first.toString());
}
// Print for debug (optional)
// for (const auto& k : keys) std::cout << k << std::endl;
// The expected order depends on your comparator logic.
// If you want to check the exact order, set it here:
// (Replace with the correct expected order if needed)
std::vector<std::string> expectedOrder = {"#19c9:e;:U;FUS;:Hce4:7,E",
"#1dadb:11;:L#1061a;FUS;:H:d,E",
"#1dae6:8;:L#1dae4;FUS;:H:d,E"};
EXPECT_EQ(keys, expectedOrder);
}
TEST_F(MappedElementTest, comparatorLargerWorkedExampleWithMap)
{
// Arrange
Data::MappedName name0("Edge123;:U;FUS;:Hce4:7,E");
Data::MappedName name1("#1dad:e;:U;FUS;:Hce4:7,E");
Data::MappedName name2("#1dadb:11;:L#1061a;FUS;:H:d,E");
Data::MappedName name3("#1dae6:8;:L#1dae4;FUS;:H:d,E");
Data::MappedName name4("Edge999;;:L#1dae4;FUS;:H:d,E");
Data::MappedName name5("g4v2;SKT;:H1234,F;:H5678:2,E;:G0(g1;SKT;:H9012,E);XTR;:H3456:2,F");
std::map<Data::MappedName, int, Data::ElementNameComparator> testMap;
testMap[name0] = 1;
testMap[name1] = 2;
testMap[name2] = 3;
testMap[name3] = 4;
testMap[name0] = 5; // Duplicate, should not affect size
testMap[name1] = 6; // Duplicate, should not affect size
testMap[name4] = 7; // New entry
testMap[name4] = 8; // Duplicate, should not affect size
testMap[name2] = 9; // Duplicate, should not affect size
testMap[name3] = 10; // Duplicate, should not affect size
testMap[name5] = 11;
// Assert: map should have 5 unique keys
EXPECT_EQ(testMap.size(), 6);
// Collect keys in order
std::vector<std::string> keys;
for (const auto& kv : testMap) {
keys.push_back(kv.first.toString());
}
// Print for debug (optional)
// for (const auto& k : keys) std::cout << k << std::endl;
// The expected order depends on your comparator logic.
// If you want to check the exact order, set it here:
// (Replace with the correct expected order if needed)
std::vector<std::string> expectedOrder = {
"Edge123;:U;FUS;:Hce4:7,E",
"Edge999;;:L#1dae4;FUS;:H:d,E",
"g4v2;SKT;:H1234,F;:H5678:2,E;:G0(g1;SKT;:H9012,E);XTR;:H3456:2,F",
"#1dad:e;:U;FUS;:Hce4:7,E",
"#1dadb:11;:L#1061a;FUS;:H:d,E",
"#1dae6:8;:L#1dae4;FUS;:H:d,E"};
EXPECT_EQ(keys, expectedOrder);
}