Skip to content

Replace library functions(isalpha & isspace) #1042

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+37 −11
Open

Replace library functions(isalpha & isspace) #1042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d2a1ef
Add IsSpace and rewrite IsNameStartChar + tests
Sulfoxide1819 Jul 25, 2025
0210bb7
Add comments for test
Sulfoxide1819 Jul 26, 2025
3b4e446
Simplify IsNameStartChar, IsSpace & IsWhiteSpace a little
Sulfoxide1819 Aug 1, 2025
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
28 changes: 17 additions & 11 deletions tinyxml2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,22 +561,28 @@ class TINYXML2_LIB XMLUtil
static char* SkipWhiteSpace( char* const p, int* curLineNumPtr ) {
return const_cast<char*>( SkipWhiteSpace( const_cast<const char*>(p), curLineNumPtr ) );
}
inline static bool IsSpace( unsigned char ch ) {
static constexpr uint64_t mask =
1ULL << 9
| 1ULL << 10
| 1ULL << 11
| 1ULL << 12
| 1ULL << 13
| 1ULL << 32;
if ( ch > 32 ) {
return false;
}
return mask >> ch & 1;
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Missing parentheses in bitwise operation. The expression mask >> ch & 1 is evaluated as mask >> (ch & 1) due to operator precedence, not (mask >> ch) & 1 as intended. This will produce incorrect results for most inputs.

Fix: return (mask >> ch) & 1;

Suggested change
return mask >> ch & 1;
return (mask >> ch) & 1;

Copilot uses AI. Check for mistakes.
}
Comment on lines +564 to +576
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Missing documentation for new function. Consider adding a comment explaining what this function does, similar to the comment on line 582 for IsNameStartChar. For example: "// Checks if a character is whitespace (tab, newline, vertical tab, form feed, carriage return, or space)"

Copilot uses AI. Check for mistakes.
Copy link
Owner

@leethomason leethomason Nov 23, 2025

Choose a reason for hiding this comment

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

This is pretty opaque - if cool - function. Why does this even work? Is there a reference to where the algo comes from?


// Anything in the high order range of UTF-8 is assumed to not be whitespace. This isn't
// correct, but simple, and usually works.
static bool IsWhiteSpace( char p ) {
return !IsUTF8Continuation(p) && isspace( static_cast<unsigned char>(p) );
return IsSpace( static_cast<unsigned char>(p) );
}

// The method checks a char for matching ':', '_', alphabetic symbols or char >= 128 by bit mask
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Trailing whitespace at end of comment line. Remove the trailing space after "bit mask".

Suggested change
// The method checks a char for matching ':', '_', alphabetic symbols or char >= 128 by bit mask
// The method checks a char for matching ':', '_', alphabetic symbols or char >= 128 by bit mask

Copilot uses AI. Check for mistakes.
inline static bool IsNameStartChar( unsigned char ch ) {
if ( ch >= 128 ) {
// This is a heuristic guess in attempt to not implement Unicode-aware isalpha()
return true;
}
if ( isalpha( ch ) ) {
return true;
}
return ch == ':' || ch == '_';
static constexpr uint64_t mask[4] = { 1ULL << 58 , 1ULL << 31 | 0x07FFFFFE07FFFFFE , ~0ULL, ~0ULL};
return ( mask[ch >> 6] >> ( ch & 63 ) ) & 1;
}

inline static bool IsNameChar( unsigned char ch ) {
Expand Down
20 changes: 20 additions & 0 deletions xmltest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2729,6 +2729,26 @@ int main( int argc, const char ** argv )
}
}
}

// ---------- Testing IsNameStartChar ----------
{
XMLUtil test;
// Tests validate key edge cases for IsNameStartChar without exhaustive coverage
XMLTest("IsNameStartChar(':')", true, test.IsNameStartChar(':'));
XMLTest("IsNameStartChar('_')", true, test.IsNameStartChar('_'));
XMLTest("IsNameStartChar('@')", false, test.IsNameStartChar('@'));
XMLTest("IsNameStartChar('A')", true, test.IsNameStartChar('A'));
XMLTest("IsNameStartChar('Z')", true, test.IsNameStartChar('Z'));
XMLTest("IsNameStartChar('[')", false, test.IsNameStartChar('['));
XMLTest("IsNameStartChar('`')", false, test.IsNameStartChar('`'));
XMLTest("IsNameStartChar('a')", true, test.IsNameStartChar('a'));
XMLTest("IsNameStartChar('z')", true, test.IsNameStartChar('z'));
XMLTest("IsNameStartChar('{')", false, test.IsNameStartChar('{'));
XMLTest("IsNameStartChar(127)", false, test.IsNameStartChar(static_cast<unsigned char>(127)));
XMLTest("IsNameStartChar(128)", true, test.IsNameStartChar(static_cast<unsigned char>(128)));
XMLTest("IsNameStartChar(255)", true, test.IsNameStartChar(static_cast<unsigned char>(255)));
}
Comment on lines +2733 to +2750
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Inconsistent indentation: this test block uses spaces for indentation, while the rest of the file uses tabs. Please convert the indentation to tabs to match the existing code style.

Suggested change
// ---------- Testing IsNameStartChar ----------
{
XMLUtil test;
// Tests validate key edge cases for IsNameStartChar without exhaustive coverage
XMLTest("IsNameStartChar(':')", true, test.IsNameStartChar(':'));
XMLTest("IsNameStartChar('_')", true, test.IsNameStartChar('_'));
XMLTest("IsNameStartChar('@')", false, test.IsNameStartChar('@'));
XMLTest("IsNameStartChar('A')", true, test.IsNameStartChar('A'));
XMLTest("IsNameStartChar('Z')", true, test.IsNameStartChar('Z'));
XMLTest("IsNameStartChar('[')", false, test.IsNameStartChar('['));
XMLTest("IsNameStartChar('`')", false, test.IsNameStartChar('`'));
XMLTest("IsNameStartChar('a')", true, test.IsNameStartChar('a'));
XMLTest("IsNameStartChar('z')", true, test.IsNameStartChar('z'));
XMLTest("IsNameStartChar('{')", false, test.IsNameStartChar('{'));
XMLTest("IsNameStartChar(127)", false, test.IsNameStartChar(static_cast<unsigned char>(127)));
XMLTest("IsNameStartChar(128)", true, test.IsNameStartChar(static_cast<unsigned char>(128)));
XMLTest("IsNameStartChar(255)", true, test.IsNameStartChar(static_cast<unsigned char>(255)));
}
// ---------- Testing IsNameStartChar ----------
{
XMLUtil test;
// Tests validate key edge cases for IsNameStartChar without exhaustive coverage
XMLTest("IsNameStartChar(':')", true, test.IsNameStartChar(':'));
XMLTest("IsNameStartChar('_')", true, test.IsNameStartChar('_'));
XMLTest("IsNameStartChar('@')", false, test.IsNameStartChar('@'));
XMLTest("IsNameStartChar('A')", true, test.IsNameStartChar('A'));
XMLTest("IsNameStartChar('Z')", true, test.IsNameStartChar('Z'));
XMLTest("IsNameStartChar('[')", false, test.IsNameStartChar('['));
XMLTest("IsNameStartChar('`')", false, test.IsNameStartChar('`'));
XMLTest("IsNameStartChar('a')", true, test.IsNameStartChar('a'));
XMLTest("IsNameStartChar('z')", true, test.IsNameStartChar('z'));
XMLTest("IsNameStartChar('{')", false, test.IsNameStartChar('{'));
XMLTest("IsNameStartChar(127)", false, test.IsNameStartChar(static_cast<unsigned char>(127)));
XMLTest("IsNameStartChar(128)", true, test.IsNameStartChar(static_cast<unsigned char>(128)));
XMLTest("IsNameStartChar(255)", true, test.IsNameStartChar(static_cast<unsigned char>(255)));
}

Copilot uses AI. Check for mistakes.


// ----------- Performance tracking --------------
{
Expand Down