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

fix reading of long lines from stdin #192

Merged
merged 1 commit into from
Oct 28, 2022
Merged

Conversation

gromgit
Copy link
Collaborator

@gromgit gromgit commented Oct 28, 2022

Also refactor slurp_file() in the process.

Addresses #191.

Also refactor slurp_file() in the process.

Addresses jpmens#191.
@jpmens jpmens merged commit a641454 into jpmens:master Oct 28, 2022
@jpmens
Copy link
Owner

jpmens commented Oct 28, 2022

Looks good to me. Thank you, Adrian!

@1480c1
Copy link
Contributor

1480c1 commented Oct 28, 2022

This commit causes an error

../jo.c:183:64: error: macro "errx" requires 3 arguments, but only 2 given
  183 |                 errx(1, "Line too large to be read into memory");
      |                                                                ^
../jo.c:51: note: macro "errx" defined here
   51 | # define errx(n, f, a)  { fprintf(stderr, f, a); exit(n); }
      | 
../jo.c:183:17: error: 'errx' undeclared (first use in this function)
  183 |                 errx(1, "Line too large to be read into memory");
      |                 ^~~~

possible fix is to replace errx() with just err() or modify the call to have some argument

@jpmens
Copy link
Owner

jpmens commented Oct 28, 2022

What platform OS/version are you building on? This builds silently for me on Darwin, and Debian11

@1480c1
Copy link
Contributor

1480c1 commented Oct 28, 2022

Windows 11 with msys2's mingw-w64 gcc and clang compiler, seems I missed a few lines in the error log, so I've added them

@jpmens
Copy link
Owner

jpmens commented Oct 28, 2022

And that macro’s been there since 2016 .. so it’s not this commit.

@1480c1
Copy link
Contributor

1480c1 commented Oct 28, 2022

As another note, there are a few other warnings

../jo.c:55: warning: "fseeko" redefined
   55 | # define fseeko fseek
      | 
In file included from ../jo.c:3:
D:/media-autobuild_suite/msys64/mingw32/include/stdio.h:672: note: this is the location of the previous definition
  672 | #define fseeko fseeko64
      | 
../jo.c:56: warning: "ftello" redefined
   56 | # define ftello ftell
      | 
D:/media-autobuild_suite/msys64/mingw32/include/stdio.h:679: note: this is the location of the previous definition
  679 | #define ftello ftello64
      | 
../jo.c:65:27: warning: ISO C does not permit named variadic macros [-Wvariadic-macros]
   65 | #define debug(format, args...) fprintf (stderr, format, args)
      |                           ^~~
../jo.c: In function 'slurp_file':
../jo.c:170:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
  170 |         if (*out_len < 0) {
      |                      ^
../jo.c: In function 'slurp_line':
../jo.c:182:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
  182 |         if (*out_len < 0) {
      |                      ^
../jo.c:183:64: error: macro "errx" requires 3 arguments, but only 2 given
  183 |                 errx(1, "Line too large to be read into memory");
      |                                                                ^
../jo.c:51: note: macro "errx" defined here
   51 | # define errx(n, f, a)  { fprintf(stderr, f, a); exit(n); }
      | 
../jo.c:183:17: error: 'errx' undeclared (first use in this function)
  183 |                 errx(1, "Line too large to be read into memory");
      |                 ^~~~
../jo.c:183:17: note: each undeclared identifier is reported only once for each function it appears in
../jo.c: In function 'utf8_from_locale':
../jo.c:548:17: warning: comparison of integer expressions of different signedness: 'size_t' {aka 'unsigned int'} and 'int' [-Wsign-compare]
  548 |         if (len == -1) {
      |                 ^~
../jo.c: In function 'locale_from_utf8':
../jo.c:581:17: warning: comparison of integer expressions of different signedness: 'size_t' {aka 'unsigned int'} and 'int' [-Wsign-compare]
  581 |         if (len == -1) {
      |                 ^~

@1480c1
Copy link
Contributor

1480c1 commented Oct 28, 2022

669f3fe shows that the instance of errx on line 183 has been added with this pr

@jpmens
Copy link
Owner

jpmens commented Oct 28, 2022

But not its definition at the top of jo.c wrapped in #ifdef.

I’m not in a position to clarify this for that platform now, but if you can and can produce a patch which fixes it, I’d appreciate it.

@1480c1
Copy link
Contributor

1480c1 commented Oct 28, 2022

https://elixir.bootlin.com/glibc/glibc-2.31/source/misc/err.h#L50

I see why it doesn't care on debian at least, errx there is defined with the third argument as variadic rather than a required argument

I will try to see if I can produce a patch to fix the issue

As another note, I am able to reproduce this with x86_64-w64-mingw32-gcc (gcc 12.2.0 "x86_64-w64-mingw32-gcc (GCC) 12.2.0") provided by archlinux

@1480c1
Copy link
Contributor

1480c1 commented Oct 28, 2022

Hi @jpmens, I've opened pr #193 to address the warnings and errors that I saw. I did not have a easy fix for the debug macro warning as it seems relatively harmless so long as you are using a gnu standard or some other standard that allows it as an extension

gromgit added a commit to gromgit/jo that referenced this pull request Nov 3, 2022
The previous attempt (jpmens#192) broke the handling of empty files.

Also added tests to catch future regressions.
gromgit added a commit to gromgit/jo that referenced this pull request Nov 3, 2022
The previous attempt (jpmens#192) broke the handling of empty files.

Also added tests to catch future regressions.

Fixes jpmens#194.
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.

3 participants