Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHandle systems where char is unsigned. #53
Conversation
|
Just a check for something I might have misunderstood. |
| @@ -613,8 +613,8 @@ struct FANSI_state FANSI_read_next(struct FANSI_state state) { | |||
|
|
|||
| // Normal ASCII characters | |||
| if(chr_val >= 0x20 && chr_val < 0x7F) state = read_ascii(state); | |||
QuLogic
Jul 25, 2018
Author
Contributor
PS, is there an off-by-one here? Should this be <= 0x7F?
PS, is there an off-by-one here? Should this be <= 0x7F?
brodieG
Jul 25, 2018
Owner
This is intentional. The 'del' character is treated the same way as 0x00 - 0x20 (zero width). I don't know why the creators of ASCII threw it at the end of the list instead of the C0 section at front (I'm sure there is a good historical reason for this).
This is intentional. The 'del' character is treated the same way as 0x00 - 0x20 (zero width). I don't know why the creators of ASCII threw it at the end of the list instead of the C0 section at front (I'm sure there is a good historical reason for this).
| @@ -116,7 +116,7 @@ struct FANSI_csi_pos FANSI_find_esc(const char * x, int what) { | |||
| int found_this = 0; | |||
|
|
|||
| // If not normal ASCII or UTF8, examine whether we need to found | |||
| if(!((x_val > 31 && x_val < 127) || x_val < 0)) { | |||
| if(!((x_val > 31 && x_val < 127) || x_val < 0 || x_val > 127)) { | |||
QuLogic
Jul 25, 2018
•
Author
Contributor
and also here with <= 127?
I'm 50/50 on this one because it seems a bit weird that 127 is left out.
and also here with <= 127?
I'm 50/50 on this one because it seems a bit weird that 127 is left out.
|
I certainly had not considered that there are systems where by default char is unsigned, or TBH even realized that the standard does not specify anything as to what char is, so there are probably problems with that here. I'm planning on making some updates to this package this week so I'll review your pull request then. I fear there may be other problems elsewhere. Please note that I'd like the pull requests made against the development branch, not the master branch (as per the contribution guidelines). Thank you for taking the time to put together a PR, and thanks for teaching me something here. |
|
Easily done, but the |
The code currently assumes that char is signed and that values>127 mean checking for < 0. This is not guaranteed by the spec, and on systems where char is unsigned, these checks fail.
Codecov Report
@@ Coverage Diff @@
## development #53 +/- ##
===============================================
- Coverage 100% 93.36% -6.64%
===============================================
Files 24 24
Lines 2044 2035 -9
===============================================
- Hits 2044 1900 -144
- Misses 0 135 +135
Continue to review full report at Codecov.
|
|
Thanks for the contribution. |
The code currently assumes that char is signed and that values>127 mean checking for < 0. This is not guaranteed by the spec, and on systems where char is unsigned, these checks fail.
As an example, see this build which fails on aarch64, armv7hl, ppc64(le) and s390x, whereas this build with the attached change is now passing.