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

Updates on int.ToString(format) #2861

Merged
merged 8 commits into from
Jan 2, 2024

Conversation

Guillermo-Santos
Copy link
Contributor

Improved Hexadecimal format code as it was changing the value of the original variable (tested only on SharpLab, since I did the change before testing on Cosmos, but I assume that if it was changing on normal C# context, then it should have been changing on Cosmos too), also added support for the decimal format and padding.

Improved Hexadecimal format code as it was changing the value of the original variable (teste only on SharpLab), also added support for the decimal format
@Guillermo-Santos Guillermo-Santos changed the title Update Int32Impl.cs Updates on int.ToString(format) Dec 28, 2023
@Guillermo-Santos
Copy link
Contributor Author

Guillermo-Santos commented Dec 28, 2023

Code used to do the test:

using System;

var a = 255;
Console.WriteLine(ToString(ref a,"X")); // Changed Version
Console.WriteLine(a); // print 255
Console.WriteLine(ToString2(ref a,"X")); // Original
Console.WriteLine(a); // print 0

/* Changed Version */
static string ToString(ref int aThis, string format)
{
    if (string.IsNullOrEmpty(format))
    {
        return "0";
    }

    string result = aThis > 0 ? string.Empty : "0";

    switch (format[0])
    {
        case 'X':
            int value = aThis;

            while (value != 0)
            {
                int remainder = value % 16;
                if (remainder < 10)
                {
                    result = remainder + result;
                }
                else
                {
                    char temp = (char)('A' + (remainder - 10));
                    result = temp + result;
                }
                
                value /= 16;
            }
            break;
        case 'D':
            result = aThis.ToString();
            break;
        default:
            return aThis.ToString();
    }

    if (format.Length > 1)
    {
        return int.TryParse(format.AsSpan(1), out int number) ? result.PadLeft(number, '0') : aThis.ToString();
    }
    else
    {
        return result;
    }
}

// Original
static string ToString2(ref int aThis, string format)
{
    if (format.Equals("X"))
    {
        string result = "";
        if(aThis == 0)
        {
            result = "0";
        }
        while (aThis != 0)
        {
            if (aThis % 16 < 10)
            {
                result = aThis % 16 + result;
            }
            else
            {
                string temp = "";
                switch (aThis % 16)
                {
                    case 10: temp = "A"; break;
                    case 11: temp = "B"; break;
                    case 12: temp = "C"; break;
                    case 13: temp = "D"; break;
                    case 14: temp = "E"; break;
                    case 15: temp = "F"; break;
                }
                result = temp + result;
            }
            aThis /= 16;
        }
        return result;
    }
    else
    {
        return aThis.ToString();
    }
}

@zarlo
Copy link
Member

zarlo commented Dec 29, 2023

since this is been worked on right now

could you make result in to a string builder so less strings are made

@Guillermo-Santos
Copy link
Contributor Author

since this is been worked on right now

could you make result in to a string builder so less strings are made

Done

@Guillermo-Santos
Copy link
Contributor Author

What does this have to do with audio?

@quajak
Copy link
Member

quajak commented Dec 30, 2023

Seems like there was an issue building the iso. This might be a random error. Could you please add some test cases to Cosmos to ensure that the new version now handles the broken cases correctly?

@Guillermo-Santos
Copy link
Contributor Author

Seems like there was an issue building the iso. This might be a random error. Could you please add some test cases to Cosmos to ensure that the new version now handles the broken cases correctly?

Should I create a new project or do I add it to IO test?

@quajak
Copy link
Member

quajak commented Dec 31, 2023

Please add them to the existing Int32 tests in the BclSystem test kernel

@Guillermo-Santos
Copy link
Contributor Author

Done

Copy link
Member

@quajak quajak left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test. Sorry for the miscommunication, we only need tests to ensure that the new version works :) not that the old one does not work

Also added a test for hex with padding
A method that was added to test the old impl, now that it will not be tested, it's reason to exists is no more.
@Guillermo-Santos
Copy link
Contributor Author

Guillermo-Santos commented Jan 1, 2024

Thanks for adding the test. Sorry for the miscommunication, we only need tests to ensure that the new version works :) not that the old one does not work

Done

@quajak quajak enabled auto-merge January 2, 2024 09:45
@quajak quajak merged commit 2d1f688 into CosmosOS:master Jan 2, 2024
2 checks passed
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.

3 participants