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

Replaced "fgets" with a "get_token" function in demo/mtest_opponent.c #500

Merged

Conversation

czurnieden
Copy link
Contributor

We shouldn't leave the clean-up of the mtest-tokens to mp_read_radix. This PR would also allow to get rid of the abominable input-sanitizing in #499

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

Tests broken, something like the following "solves the problem"^TM (not sure what other problems it introduces ;) )

diff --git a/demo/mtest_opponent.c b/demo/mtest_opponent.c
index 9f80e65..dbf8745 100644
--- a/demo/mtest_opponent.c
+++ b/demo/mtest_opponent.c
@@ -21,3 +21,3 @@ static char *s_mp_get_token(char *s, int size, FILE *stream)
    char *s_bar = s;
-   int c;
+   int c, eol_hit = 0;
 
@@ -35,4 +35,9 @@ static char *s_mp_get_token(char *s, int size, FILE *stream)
       if ((c == '\n')  || (c == '\r')) {
+         eol_hit = 1;
          continue;
       }
+      if (eol_hit) {
+         ungetc(c, stream);
+         break;
+      }
       *s_bar++ = c;
@@ -110,3 +115,2 @@ static int mtest_opponent(void)
       GET_TOKEN(cmd, 4095, stdin);
-      cmd[strlen(cmd) - 1u] = '\0';
       printf("%-6s ]\r", cmd);

@czurnieden
Copy link
Contributor Author

Tests broken

I always try not to leave my PRs broken but Travis took way too long to start that day^Wnight so I went to bed. Sorry!
(Why didn't it come up when I run --test-vs-mtest locally? That's annoying!)

not sure what other problems it introduces

Good question. I'll take a deeper look this time.

Thanks!

@czurnieden
Copy link
Contributor Author

Does seem to work, now that s_mp_get_token stops at the right time.
Spot-checked input/output (If the names gets over correctly and the numbers for sub_d).

Why the local tests did fail before PR is still a mystery to me, because now they do. But I also forgot to remove cmd[strlen(cmd) - 1u] = '\0';, so it was most likely me to blame here.

There is still a non-technical problem left: because of the age of this part (read_radix) of LTM there might be many uses of the undocumented feature of read_radix to accept linebreaks at the end of the input string. This PR won't break them but if this PR gets included I can and will remove the input cleaning in #499 which will break those builds.
I admit I'm a bit torn here.

@czurnieden czurnieden force-pushed the clean_strings_for_mtest_opponent branch from 2ae6533 to 24ac0de Compare December 29, 2020 19:49
* handle buffer full case
* display error reason of `s_mp_get_token()`
* display name of variables when `draw()`ing on error
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