Skip to content

Conversation

@IvanLeu
Copy link
Contributor

@IvanLeu IvanLeu commented May 11, 2024

Function strinToInt that returns int64_t is implemented using efficient algorithm of converting string to int. For the sake of initial string data safety I used helper c-style string(But it can be implemented without any helper strings by traversing string data field). Also the sign of the number is considered. Error handling is implemented using this frameworks error handling tools. Also new error code was added for better debugging. As helper function was added function stringConvertToCharArr that converts TString type to c-style string type.

cstring.h Outdated
char stringCharToUpper(char c);

int stringCharToInt(char c);
int64_t stringCharToInt64(char c);
Copy link
Owner

Choose a reason for hiding this comment

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

this is a kind of duplication of the previous function, because int can always be implicitly converted to int64_t

cstring.h Outdated
Comment on lines 677 to 683
if(res == NULL) {
setError(ERR_NULL_POINTER);
}

if (isError()) {
return res;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if(res == NULL) {
setError(ERR_NULL_POINTER);
}
if (isError()) {
return res;
}
if (res == NULL) {
setError(ERR_NULL_POINTER);
return NULL;
}

cstring.h Outdated

char* str = stringConvertToCharArr(s);

if(str[0] == '-'){
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Segfault when len = 0
  2. Please, check code style convention
  3. It is not necessary to allocate a C-string, better use s.data and s.size

cstring.h Outdated
Comment on lines 407 to 408
// check for integer overflow
if(val < 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

See this how to prevent signed overflow
Now it is UB

@IvanLeu IvanLeu requested a review from Tnirpps May 16, 2024 04:23
cstring.h Outdated
for (; i < s.size; i++) {
int64_t digit = stringCharToInt(s.data[i]);

if (val * 10 > INT64_MAX - digit) {
Copy link
Owner

Choose a reason for hiding this comment

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

Everything is good but this still can cause overflow val * 10. It'l be better to check smth like this:

val > (INT64_MAX - digit) / 10

be careful with rounding, it requires divisibility check

@IvanLeu IvanLeu requested a review from Tnirpps May 18, 2024 09:44
int stringCharToInt(char c) {
if ('0' <= c && c <= '9')
return c - '0';
setError(ERR_INVALID_NUMBER_REPR);
Copy link
Owner

Choose a reason for hiding this comment

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

If function can change error state, it have to clear error in the begin, to prevent smth like that:

// err = no_err
foo()                              // set err to smth, but result wasn't checked
...
//err = some_err
bar()                              // no error actually happened but flag wasn't cleaned
if (err == some_err) {             // check error in bar() result
    ...                            // but goes here because err after foo()
}

Copy link
Owner

Choose a reason for hiding this comment

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

although it is better to always check the error after each function call

cstring.h Outdated
Comment on lines 394 to 396
if (val > (INT64_MAX - digit) / 10) {
setError(ERR_NUMBER_OVERFLOW);
return val;
Copy link
Owner

Choose a reason for hiding this comment

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

Such a nice code!
just change to >= and add test for stringToInt(INT64_MAX)

Copy link
Owner

Choose a reason for hiding this comment

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

it will probably overflow with INT64_MIN due to the asymmetric range but you may just leave TODO

@IvanLeu IvanLeu requested a review from Tnirpps June 2, 2024 11:34
Copy link
Owner

@Tnirpps Tnirpps left a comment

Choose a reason for hiding this comment

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

Nice

@Tnirpps Tnirpps merged commit 57592c9 into Tnirpps:main Jun 4, 2024
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.

2 participants