-
Notifications
You must be signed in to change notification settings - Fork 442
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
[FEAT] Add encoding module in lib_ccxr
#1628
[FEAT] Add encoding module in lib_ccxr
#1628
Conversation
A module for working with different kinds of text encoding formats
fn latin1_to_line21(_c: Latin1Char) -> Line21Char { | ||
todo!() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 todo
remaining
Researching on this one
@PunitLodha @elbertronnie Any idea about it, This function is not included in C but rust match(line 417) needed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it is not required in C, I wanted all forms to be convertable to all other forms of String simply because of completeness. Implementing this would take some time which is why I left a todo in there to do it later.
If you want to implement it you can do it by inverting the logic in line21_to_latin1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it is not required in C, I wanted all forms to be convertable to all other forms of String simply because of completeness. Implementing this would take some time which is why I left a todo in there to do it later.
Yeah exactly @elbertronnie , I removed some todo from your code, just left with this one only
If you want to implement it you can do it by inverting the logic in line21_to_latin1.
I am trying to do that only
} | ||
|
||
fn line21_to_utf8(c: Line21Char) -> char { | ||
0x80 as char |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation feels incorrect. It will always return 0x80. Is this how it is defined in C?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh Sorry that is mistake
} | ||
|
||
fn ucs2_to_line21(c: Ucs2Char) -> Line21Char { | ||
if c < 0x80 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is also wrong. It should be having inverted logic of line21_to_ucs2
. But to create this, you will require latin1_to_line21
first. I would recommend to either implement this or put a todo here. It is not used in C anyway.
I know that I wrote this and I don't remember what I was thinking while writing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done it @elbertronnie by reversing the function
@PunitLodha @elbertronnie Can you now see it & review |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Closes #1554
Added a module in
lib_ccxr
for working with different kinds of text encoding formats.This PR adds types used for representing Strings encoded in a certain format like Line 21 or UCS-2 and the ability to interconvert between them.