tenox7 / ttyplot

a realtime plotting utility for terminal/console with data input from stdin
Apache License 2.0
957 stars 42 forks source link

Potentially incorrect usage of `optind`. #103

Closed MIvanchev closed 7 months ago

MIvanchev commented 7 months ago

This should fix #102.

MIvanchev commented 7 months ago

@hartwork good idea!

hartwork commented 7 months ago

@MIvanchev thanks for the adjustment!

I found this sentence in man optind:

A program that scans multiple argument vectors, or rescans the same vector more than once, and wants to make use of GNU extensions such as '+' and '-' at the start of optstring, or changes the value of POSIXLY_CORRECT between scans, must reinitialize getopt() by resetting optind to 0, rather than the traditional value of 1. (Resetting to 0 forces the invocation of an internal initialization routine that rechecks POSIXLY_CORRECT and checks for GNU extensions in optstring.)

Can you confirm that we do not need/have any of that?

MIvanchev commented 7 months ago

I don't think we need, but it seems like MacOS' getopt has different semantics regarding optind: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/getopt.3.html

hartwork commented 7 months ago

I don't think we need, but it seems like MacOS' getopt has different semantics regarding optind: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/getopt.3.html

What difference did you observe in detail?

I notice now that "rescans the same vector more than once" is exactly our case.

Do we need one version of the code for GNU getopt and one version for non-GNU maybe? Just thinking aloud.

hartwork commented 7 months ago

@MIvanchev PS: Also thinking aloud: maybe the most portable fix would be to merge the two scans, to avoid rescanning. What was the key idea behind separating the two scans?

MIvanchev commented 7 months ago

What was the key idea behind separating the two scans?

To avoid doing anything, including scanf and printf, if the user has requested -v or -h and to have a well-defined order for -v and -h.

hartwork commented 7 months ago

I don't think we need, but it seems like MacOS' getopt has different semantics regarding optind: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/getopt.3.html

What difference did you observe in detail?

I found this now in the macOS man page that you linked above:

In order to use getopt() to evaluate multiple sets of arguments, or to evaluate a single set of arguments multiple times, the variable optreset must be set to 1 before the second and each additional set of calls to getopt(), and the variable optind must be reinitialized.

I see.

hartwork commented 7 months ago

What was the key idea behind separating the two scans?

To avoid doing anything, including scanf and printf, if the user has requested -v or -h

@MIvanchev so an optimization rather than a strong need, okay.

and to have a well-defined order for -v and -h.

How would the order not be well-defined without? What am I missing?

edgar-bonet commented 7 months ago

The second scan of the options does nothing other than set some variables, call atof() and snprintf(). There is no harm in doing this if the user asks for -v or -h. Merging the two scans would not only completely remove this issue, it would also make the code simpler.

MIvanchev commented 7 months ago

@edgar-bonet this is true but I suggest not to include a refactoring in the scope of the MR unless we want to derail the discussion in hundreds of comments now, there are other pressing matters :)

edgar-bonet commented 7 months ago

@MIvanchev: I bet merging the two scans would solve the problem with far less comments than the current approach.

MIvanchev commented 7 months ago

Done :+1: @tenox7 @edgar-bonet be aware of the force push if you've previously cloned a copy.

tenox7 commented 7 months ago

Tested on FreeBSD and MacOS. LGTM. Thank you!!!!!!