Port-StringTools #56

MakoEnergy · 2019-07-22T01:26:43Z

A port of the StringTools from the monolithic repo.

Sqeaky · 2019-08-05T06:45:05Z

include/StringTools.h

+    /// @brief Checks if a character is a space.
+    /// @param ToCheck The character to be checked.
+    /// @return Returns true if the character is a space, false otherwise.
+    Boole MEZZ_LIB IsSpace(const Char8 ToCheck);


Would it make sense to have signed and unsigned versions of this? I get that Cha8 is our type and we will set it appropriately, but someone using and external type could benefit.

EDIT - Same question applies to any function accepting a char.

That would be an enormous amount of extra boilerplate and I'm not sure what the use case for it would be.

Nevermind. I suggested it for the cornercase where the game dev cared about signedness of their chars.

I have made all those functions templates with a type trait for sanity.

Sqeaky · 2019-08-09T00:17:21Z

include/StringTools.h

+    Boole MEZZ_LIB IsTab(const Char8 ToCheck);
+    /// @brief Checks if a character is a newline.
+    /// @param ToCheck The character to be checked.
+    /// @return Returns true if the character is a newline, false otherwise.


What does this do with a \r? It is part of a newline on some platforms.

Looking at the source, the function checks for both \r and \n.

I was implying that should be in the docs.

Sqeaky · 2019-08-09T02:19:38Z

include/StringTools.h

+    /// @brief Gets a copy of a String with the first letter of each word upper cased and other letters lower cased.
+    /// @param Source The String that will be the basis for the modified String.
+    /// @return Returns a copy of the String that has been modified to be in Camel Case.
+    String MEZZ_LIB CamelCaseCopy(String Source);


This should be PascalCase or DragonCase. See #57

Sqeaky · 2019-08-09T02:38:58Z

include/StringTools.h

+    /// @param Source The String that will be the basis for the modified String.
+    /// @param Left Whether or not to trim the left side of the String.
+    /// @param Right Whether or not to trim the right side of the String.
+    String MEZZ_LIB TrimCopy(String Source, const Boole Left = true, const Boole Right = true);


I am used to seeing this as 3 different functions instead of parameters on one. LeftTrim, RightTrim and Trim.

Also a version of this could operate on iterator ranges like std::erase and return an iterator to the new end of the range or the end of the erased range,.

I have modified it to have 3 versions. The Non-copy version operates like std::erase as you suggested.

Sqeaky · 2019-08-09T02:40:22Z

include/StringTools.h

+    void MEZZ_LIB RemoveDuplicateWhitespaces(String& Source);
+    /// @brief Gets a copy of a string with all duplicate whitespaces removed.
+    /// @param Source The String that will be the basis for the modified String.
+    String MEZZ_LIB RemoveDuplicateWhitespacesCopy(String Source);


Again there could be an iterator range version like std::erase.

Sqeaky · 2019-08-09T02:52:00Z

include/StringTools.h

+    /// @param Delims The characters to look for and use as split points in the source String.
+    /// @param MaxSplits The maximum number of splits to perform on this String. Value of zero means unlimited splits.
+    /// @return Returns a vector containing all the substrings generated from the source String.
+    StringVector MEZZ_LIB Split(const String& Source, const String& Delims = " \t\n", const Whole MaxSplits = 0);


Do we have a matching: String Join(StringVector ToJoin, const String& Delim=" ");

Js, Ruby, Python, and PHP all have some function to match their split/explode functions that join groups into a single string following a simple pattern of ToJoin[0] + Delim +ToJoin[1] + Delim ... ToJoin[size-1];

Sqeaky · 2019-08-09T04:07:49Z

include/StringTools.h

+    ///////////////////////////////////////////////////////////////////////////////
+    // Convert-From-String Functions
+
+    /// @brief Converts a String to any.


I feel like this needs the phrase "Lexicographical Conversion"

Since you highlighted multiple parts here....do you mean the section header or the method brief doc?

Sqeaky · 2019-08-09T04:08:49Z

include/StringTools.h

+        StringStream Converter;
+        Converter << ToConvert;
+        return Converter.str();
+    }


Should This delegate to std::to_string?

No. std::to_string is intended for numeric conversions. Whereas this will work with any type that has a streaming operator.

Sqeaky · 2019-08-09T05:47:11Z

src/StringTools.cpp

+        const String Delims = " \t\r";
+        if( Left ) {
+            size_t Forward = 0;
+            while( Forward < Source.size() )


This doesn't seem like a reasonable implementation. Why not use std::find_first_of or std::find_if_not to find the range for std::erase.

Also, this is just conjecture, but I cannot imagine that the branching helps with optimization, if this were just a call RightTrim and LeftTrim then the game dev could decide which of the 3 to call and just never branch.

EDIT - Strings have a find_first_not_of on them. This could make ltrim super easy to implement.

The implementation has been updated as per a previous review comment.

Sqeaky · 2019-08-09T06:02:27Z

src/StringTools.cpp

+             CurrIndex = Source.find_first_of(Delims,CurrIndex,2) )
+        {
+            size_t EndIndex = CurrIndex;
+            Char8 CurrWhite = Source[EndIndex];


Would this work on a string with several alternating tabs and spaces in between words?

No, nor was it meant to when it was written. It could be updated to handle such cases...although I'm curious what the use case for that would be and which of the alternating whitespace characters should be prioritized when replacing the sequence with one character.

Sqeaky · 2019-08-09T06:06:00Z

src/StringTools.cpp

+    Real ConvertHexToColourChannel(const StringView Hex)
+    {
+        if( Hex.size() != 2 ) {
+            String ExceptionString("Hex code requires 2 characters to express a Colour channel.");


CSS has color hex code triplets. It just omits the lower precision number. If a single digit is passed we could simply duplicate that digit before resuming.

Sqeaky · 2019-08-09T06:08:46Z

src/StringTools.cpp

+            return Ret;
+        }
+
+        Ret.reserve( MaxSplits ? MaxSplits + 1 : 10 );


What is the use case for limiting the amount of splits? I have not seen this before.

EDIT - I get this is just reserving and the capacity, but it I clicked add comment before I finished thinking about where to put this.

The default value is no maximum. I want to say I have used the maximum somewhere in the Mezzanine, but I can't recall where. I think this is a relic from Ogre, but unlike other Ogre relics this is doing no harm.

Sqeaky · 2019-08-09T06:18:23Z

src/StringTools.cpp

+    Boole ConvertToBool(const StringView ToConvert, const Boole Default)
+    {
+        auto Checker = [ToConvert](const StringView Str) {
+            if( Str.size() == ToConvert.size() ) {


This would be better as a call to std::equal which likely does a range length check on its own.

Given it's parameters, I don't see how it could:
https://en.cppreference.com/w/cpp/algorithm/equal

Sqeaky · 2019-08-09T06:20:26Z

src/StringTools.cpp

+    }
+
+    template<>
+    UInt8 ConvertFromString<UInt8>(const StringView ToConvert)


Why is this specialized?

Implicit conversions turn it into an int, which produces unexpected results when interacting with streams. The specializations work around that.

Sqeaky · 2019-08-12T02:59:45Z

test/StringToolsTests.h

+        TEST_EQUAL("IsLowerHexLetter(const_Char8)-Fail",false,StringTools::IsLowerHexLetter('g'));
+        TEST_EQUAL("IsUpperHexLetter(const_Char8)-Pass",true,StringTools::IsUpperHexLetter('F'));
+        TEST_EQUAL("IsUpperHexLetter(const_Char8)-Fail",false,StringTools::IsUpperHexLetter('1'));
+        TEST_EQUAL("IsHexLetter(const_Char8)-Pass",true,StringTools::IsHexLetter('B'));


Should add a lowercase 'b' test expecting a true.

Sqeaky · 2019-08-12T03:07:25Z

test/StringToolsTests.h

+
+    {//String Manipulation
+        String ToUpperString("This is typed in a reasonable, soft-spoken tone.");
+        String ToUpperResult("THIS IS TYPED IN A REASONABLE, SOFT-SPOKEN TONE.");


/Nit Results for test should be const, the strings to be copied could be too.

Not sure I got them all but I made a large number of the source strings const.

Sqeaky · 2019-08-12T03:20:17Z

test/StringToolsTests.h

+        TEST_EQUAL_EPSILON("ConvertHexToColourChannel(const_StringView)-Fourth",
+                           Real(6.0 / 255.0),StringTools::ConvertHexToColourChannel(HexStr4));
+
+        Real ChannelVal1 = 0.8675f;//221.2125


/Nit - The fractions above are more readable in my opinion.

Sqeaky · 2019-08-12T03:57:37Z

test/StringToolsTests.h

+        StringVector ISplitResult = StringTools::Split(SplitSourceString,"i",5);// Should normally split 7 times.
+        TEST_EQUAL("Split(const_String&,const_String&,const_Whole)-DefaultCount",
+                   size_t(11),DefaultSplitResult.size());
+        TEST_EQUAL("Split(const_String&,const_String&,const_Whole)-DefaultElement1",


This mixes two different tests. It is a minor nit, but 2 sets of tests, one testing the split char and another testing split limit would better follow best practices. That said, this is simple enough its not a real issue.

There are two sets of tests for Split as is. One uses the default value for splits and the other doesn't. Although the one that doesn't (the ISplit tests) also happens to use a different delimiter. I guess it's still not ideal, but we do have one default value test and one non-default value test.

Sqeaky · 2019-08-12T04:06:37Z

test/StringToolsTests.h

+        String ThirdSource("Test\\Data\\Results.txt");
+        String FourthSource("*Star*");
+
+        String FirstPattern("Test/Data/*.txt");


Pattern names that indicate what they match would improve readabilty. This could "PatternMatchesFirst" or "PatternMatchesUnixTextFile" for example.

Sqeaky · 2019-08-12T04:27:20Z

test/StringToolsTests.h

+        TEST_EQUAL("ConvertToBool(const_StringView,const_Boole)-Default",
+                   true,StringTools::ConvertToBool("Yes"));
+        TEST_EQUAL("ConvertToBool(const_StringView,const_Boole)-Gibberish",
+                   false,StringTools::ConvertToBool("Gibberish"));


I think a ("gibberish", true) test should be added to specifically test the defaulting behavior.

Replaced all Size Modifying String Manipulations that accepted a String with versions that accept an iterator range. Renamed CamelCase to Pascal Case. Added minor documentation. Changed String parameters in Split to StringViews.

MakoEnergy · 2019-08-19T00:37:12Z

Latest round of tests only failed because of CountedPtr TestWarns.

MakoEnergy · 2019-08-19T00:59:57Z

More TestWarn "failures".

A port of the StringTools from the monolithic repo.

24ad451

MakoEnergy added enhancement mezzanine port The code for this exists in the old Mezzanine and should be copied and modified from there. labels Jul 22, 2019

MakoEnergy requested a review from Sqeaky July 22, 2019 01:26

MakoEnergy self-assigned this Jul 22, 2019

MakoEnergy and others added 4 commits July 21, 2019 20:50

A few minor fixes from Clang reports.

a36b34a

Replaced the <charconv> implementations because some compilers suck.

b853caf

Couple more fixes for Clang/MSVC.

fb521ce

Merge branch 'master' into Port-StringTools

f685cd4

Sqeaky reviewed Aug 5, 2019

View reviewed changes

Sqeaky reviewed Aug 9, 2019

View reviewed changes

Sqeaky reviewed Aug 12, 2019

View reviewed changes

MakoEnergy added 2 commits August 17, 2019 04:12

Incremental updates to the StringTools

be76a25

Replaced all Size Modifying String Manipulations that accepted a String with versions that accept an iterator range. Renamed CamelCase to Pascal Case. Added minor documentation. Changed String parameters in Split to StringViews.

Clang fixes because of signed/unsigned comparisons.

dbadedb

Changed the char checking methods to be SFINAE template methods.

48f891c

MakoEnergy added 2 commits August 18, 2019 19:39

Added Join and renamed some pattern strings.

32e7742

Source -> Sources. Stoopid Clang.

8fde8ee

MakoEnergy added 2 commits August 19, 2019 00:24

Made signed char work with is_char.

00888f0

Fixed an issue with string size reservations.

bc8b611

MakoEnergy merged commit b0dcb1e into master Aug 19, 2019

MakoEnergy deleted the Port-StringTools branch August 19, 2019 05:51

MakoEnergy mentioned this pull request Aug 19, 2019

Port StringTools #20

Closed

Port-StringTools #56

Port-StringTools #56

Conversation

MakoEnergy commented Jul 22, 2019

Sqeaky Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sqeaky Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sqeaky Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sqeaky Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakoEnergy commented Aug 19, 2019

MakoEnergy commented Aug 19, 2019

Sqeaky Aug 5, 2019 •

edited

Loading

Sqeaky Aug 9, 2019 •

edited

Loading

Sqeaky Aug 9, 2019 •

edited

Loading

Sqeaky Aug 12, 2019 •

edited

Loading