Watch implementation changes

[arthurbacci]

DON'T MERGE NOW!
Please review the code and test it. More commits will most likely be needed before the merge since I didn't test anything.

[takusuman]

I am having some trouble with my box for now, but I will test it as soon as possible.

[takusuman]

@arthurbacci It seems to work fairly well.
How does it go with optimizations? I haven't tested -O3 or -funroll-loops yet.

[arthurbacci]

"This is an old sock project";
Proceeds to replace all /* */s by //s

[arthurbacci]

@arthurbacci It seems to work fairly well. How does it go with optimizations? I haven't tested -O3 or -funroll-loops yet.

Needs testing

[takusuman]

@arthurbacci It seems to work fairly well. How does it go with optimizations? I haven't tested -O3 or -funroll-loops yet.

Needs testing

"Testing Heirloom" for me usually means using it everyday, but I think we could fix some annotations I've made before.

[takusuman]

Already reviewed the code, made some annotations at:
https://github.com/Projeto-Pindorama/heirloom-ng/blob/watch-implementation-changes/watch/watch.c#L164-L170
https://github.com/Projeto-Pindorama/heirloom-ng/blob/watch-implementation-changes/watch/watch.c#L186-L193

I think we could also rework perror(), since it doesn't work properly and just prints "ERROR:" without anything else about the error itself. Maybe it's errno fault?
But this would fall into libcommon, where it is defined, not here.
We could also fix the comments and make it C99, so it will compile on pretty much anything (like Heirloom NG was meant to).
About systems that were not POSIX.1b (IEEE Std 1003.1b-1993) yet --- for some reason, such as being old and we're still needing to support it ---, an #ifdef that limits watch to use a integer number as time for the vulgar sleep() function shall be enough --- anyway, I think everyone here already supports the POSIX standard from 1993, eh? Well, so nevermind, just an observation.

[arthurbacci]

I believe perror() is from the C standard library. The one that seems to be specific to heirloom is prerror()

[arthurbacci]

The code was previously using prerror(), but in my additions i used perror(). prerror() seems to require an integer argument (errno), but it doesn't require any string in order to print the specific issue.

[arthurbacci]

FIXME: When ones use "-n 0.1", it actually prints "0.100000000" instead of 0.1 or even 0.10. That could be fixed --- although not being a real problem --- aiming for less visual pollution on the status bar. Maybe this could be fixed by multiplying the interval by a nanosecond ratio on nanosleep()?

nanosleep() requires seconds and nanoseconds separately. In order to do what you want, you would need to either: print (double/float)seconds + (double/float)nanoseconds / 10e9, formatted with %f, or somehow remove the trailing zeroes from the nanoseconds.
But I think the best way would be to just print the argument the user gave the program instead. The only problem with this is checking either the parsing of the argument is working as intended or not. If you're going to implement that way please check if the user argument is valid during the entire program.

[arthurbacci]

I think that a case involving left_len and right_len, which contain the "Every (int).(int) second(s): (string)" and "hostname: date". respectively, is improbable, so this is just a pure formality in case of one's curses implementation having a faulty snprintf(). EXIT_FAILURE could be a more "frightening" code that gives a clue about the seriousness of this error on his/her system curses implementation.

snprintf() is not part of curses, but of the C Standard Library (since C99). In versions before C99 it may be implemented somewhere else, but definitely not curses. Considering the wide usage of this function, the user should be warned if his implementation has faults, since this could cause catastrophic consequences.

When not consideration a faulty impl., I don't really know if runtime issues can cause snprintf to err or not. But I couldn't find any resources saying that it can't. In old versions of gnu's libc, for example, it would return a negative value if truncation happens. I don't know what you expect more to be done, but since a negative (or 0, it makes no sense for nothing to be print) can't represent the expected behaviour in any way, I think the best is to panic and exit.

[arthurbacci]

FIXME: When ones use "-n 0.1", it actually prints "0.100000000" instead of 0.1 or even 0.10. That could be fixed --- although not being a real problem --- aiming for less visual pollution on the status bar. Maybe this could be fixed by multiplying the interval by a nanosecond ratio on nanosleep()?

nanosleep() requires seconds and nanoseconds separately. In order to do what you want, you would need to either: print (double/float)seconds + (double/float)nanoseconds / 10e9, formatted with %f, or somehow remove the trailing zeroes from the nanoseconds. But I think the best way would be to just print the argument the user gave the program instead. The only problem with this is checking either the parsing of the argument is working as intended or not. If you're going to implement that way please check if the user argument is valid during the entire program.

After implementing it, pls. request a review from me.

[takusuman]

I believe perror() is from the C standard library. The one that seems to be specific to heirloom is prerror()

Yes, we've implemented back in 2022 if I'm not erred.

The code was previously using prerror(), but in my additions i used perror(). perror() seems to require an integer argument (errno), but it doesn't require any string in order to print the specific issue.

It's weird the fact that it just prints "ERROR:" anyway, isn't this our fault?

[...] or somehow remove the trailing zeroes from the nanoseconds.

Couldn't this be achieved by using some specific option at printf()? I don't think that a more complex solution than trimming everything is worth.

But I think the best way would be to just print the argument the user gave the program instead. The only problem with this is checking either the parsing of the argument is working as intended or not. If you're going to implement that way please check if the user argument is valid during the entire program.

Well, I think we already do this sort of check back in getopt() when parsing the time, so we doesn't need to be preoccupied with re-checking the input.

snprintf() is not part of curses, but of the C Standard Library (since C99). In versions before C99 it may be implemented somewhere else, but definitely not curses.

I will correct this later at the comment. Thanks.

I don't know what you expect more to be done, but since a negative (or 0, it makes no sense for nothing to be print) can't represent the expected behaviour in any way, I think the best is to panic and exit.

Just change EXIT_FAILURE to 255, which usually means that a severe error occurred in the UNIX tradition.

[arthurbacci]

Yes, we've implemented back in 2022 if I'm not erred.

Was there any old function from heirloom with that name or you decided to create it?

It's weird the fact that it just prints "ERROR:" anyway, isn't this our fault?

ERRATA: prerror() needs a number. perror("amongus") is expected to print amongus: READABLE ERRNO in stderr. It doesn't?

Couldn't this be achieved by using some specific option at printf()? I don't think that a more complex solution than trimming everything is worth.

I was thinking about it but couldn't recall any.

But I think the best way would be to just print the argument the user gave the program instead. The only problem with this is checking either the parsing of the argument is working as intended or not. If you're going to implement that way please check if the user argument is valid during the entire program.

Well, I think we already do this sort of check back in getopt() when parsing the time, so we doesn't need to be preoccupied with re-checking the input.

The problem isn't checking the input itself, but making sure getopt() doesn't free or replace it in any way. I don't think that's the case

Just change EXIT_FAILURE to 255, which usually means that a severe error occurred in the UNIX tradition.

Feel free to do so.

[takusuman]

How can I request a review? Damn.

[arthurbacci]

Seems fine

[arthurbacci]

o k .
Still, the code doesn't comform with C90.

[arthurbacci]

Doesn't seem very fine. Some changes would be good

[arthurbacci]

Why?

[takusuman]

o k . Still, the code doesn't comform with C90.

I know this is not so urgent for now, but what is missing in this code to it conform with C90?

[arthurbacci]

o k . Still, the code doesn't comform with C90.

I know this is not so urgent for now, but what is missing in this code to it conform with C90?

Several things

[arthurbacci]

Seems right

[arthurbacci]

Seems right

[arthurbacci]

I believe it's ready for merging

[arthurbacci]

Seems right

[takusuman]

I will just look for memory leaks before merging.

[takusuman]

Although Valgrind reports memory leaks at newterm() (line 153) and refresh() (line 244), it's perfectly normal to it give some false positives about memory leaks on ncurses functions, according to its F.A.Q.: https://invisible-island.net/ncurses/ncurses.faq.html#config_leaks

We can conclude that it's good to merge and working flawlessly well.

[takusuman]

Merged!