Conversation
Codecov Report
@@ Coverage Diff @@
## main #132 +/- ##
==========================================
- Coverage 18.58% 18.55% -0.04%
==========================================
Files 126 126
Lines 13540 13562 +22
==========================================
Hits 2517 2517
- Misses 11023 11045 +22
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
HexDecimal
left a comment
HexDecimal
left a comment
There was a problem hiding this comment.
I didn't realize how non-obvious it was that everything above the // New UTF-8 parser. line is something to be avoided.
It's very unfortunate that you performed this work on your main branch instead of a separate branch. I can't safely push changes to the PR in this state. You will also be unable to make a 2nd PR until this is fixed.
I don't like the unversioned print functions. They should at least have something such as rgb as in TCOD_printn_rgb and TCOD_PrintParamsRGB to at least define that this version is using the RGB color types.
Tests will need to be added to tests/test_printing.cpp to satisfy Codecov.
If you're ever done working on this PR then it's possible for me to merge this now. I'll have an easier time resolving these issues since I'm more familiar with the library and the issues are relatively small.
src/libtcod/console_printing.h
Outdated
| TCOD_PUBLIC int TCOD_printn( | ||
| TCOD_Console* __restrict console, | ||
| TCOD_PrintParams params, | ||
| char* strsrc, |
There was a problem hiding this comment.
Why is strsrc not const?
There was a problem hiding this comment.
This might also be misspelled? I assume it's supposed to be src. It's also wrong for a sized array to come before the parameter which determines it's size. Even though there are functions in the standard which do that, it's a known issue which you should avoid.
src/libtcod/console_printing.c
Outdated
| TCOD_PrintParams params, | ||
| const char* fmt, | ||
| va_list args) { | ||
| int err = TCOD_console_print_internal(console, params.x, params.y, params.width, params.height, params.flag, params.alignment, TCOD_console_vsprint(fmt, args), true, false); |
There was a problem hiding this comment.
TCOD_console_print_internal is the deprecated EASCII series of functions. No new code should use any internal function other than printn_internal_.
There was a problem hiding this comment.
Also TCOD_console_vsprint is not as reentrant as it looks, and should be avoided. vprintf_internal_ handles this case.
src/libtcod/console_printing.c
Outdated
| char* strbuf = malloc(n); | ||
| strncpy(strbuf, strsrc, n); | ||
| int err = TCOD_console_print_internal(console, params.x, params.y, params.width, params.height, params.flag, params.alignment, strbuf, true, false); | ||
| free(strbuf); |
There was a problem hiding this comment.
printn functions should no do any allocations. The string and size should be passed to printn_internal_.
|
Ahh, my bad. I haven't worked on this library in a while. I'll go ahead and fix those issues, thank you for pointing them out |
|
Also, I somehow didn't realize I was pushing to the main branch of my fork. I can move it to a different fork with a different branch, if you think that'd work best |
|
Move this to a topic branch and then I'll start pushing changes to that branch. Most of the issues are small and should be easy for me to fix. |
2 participants
Adds "TCOD_printf", "TCOD_printn", and "TCOD_vprintf" (Along with a PrintParams struct) to console_printing. Also fixes some typos