add gitignore, fix c99 warnings #15
Conversation
metamath.c
Outdated
| @@ -1138,7 +1138,7 @@ void command(int argc, char *argv[]) | |||
| } | |||
| } | |||
| /* Do the operating system command */ | |||
| (void)system(str1); | |||
| (void)!system(str1); | |||
There was a problem hiding this comment.
The result of system() is not used. To suppress warnings, and to indicate this is deliberate, a cast to void is performed. https://stackoverflow.com/questions/689677/why-cast-unused-return-values-to-void
If there is a reason to invert the result this should be noted in a comment.
There was a problem hiding this comment.
Will do. The reason is because of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 : gcc does not respect the cast-to-void trick, and it is not clear whether this is intentional on their part but it doesn't look like it's going to be fixed any time soon.
An alternative trick is if(system(str1)) {}, not sure if that's any better.
There was a problem hiding this comment.
Comment looks fine.
In the end, this is a specific code style, a rule that should be gathered in the README (or similar location)
There was a problem hiding this comment.
If we want a coding style, perhaps we could get ambitious and shoot for "don't ignore the return value from system" (because it could fail if out of memory, out of file descriptors, etc)?
I mean, we have gotten by with this code for a long time, so I'm not saying this is necessarily our top priority, just that.... well putting in a simple error check might not kill us.
There was a problem hiding this comment.
@jkingdon In this case, the call to system is because the user explicitly attempted to call an external program. The program is basically acting like bash here, so failures are expected and should not crash the program, although arguably they should be displayed (even though bash itself doesn't, it just stashes the result in the $? variable).
There was a problem hiding this comment.
Unless we have something analogous to $? I'd say display a message, yes.
| @@ -0,0 +1,14 @@ | |||
| *.o | |||
There was a problem hiding this comment.
Metamath is not only distributed via GitHub but also via an archive tar file. In the latter case, a gitignore may overwrite an existing one, because the tar-file contents could be extracted within an existing git environment.
When I extract an archive, I simply do not expect a gitignore be present. That this file is usually invisible, adds to possible complications.
There was a problem hiding this comment.
In this case, we should adjust the archival process to not include the .gitignore file (or the .git folder, for that matter). Putting the sources in a src folder may help with this.
There was a problem hiding this comment.
I propose merging this with the .gitignore as is. Currently archive generation is on hiatus since Norm isn't around to keep it running anymore, and when it returns it will be powered by github actions or such and so we will be able to adjust it as necessary.
There was a problem hiding this comment.
This means we abandon the distribution via a tar file for now. IMO GitHub is good enough to be the primary (and possibly only) distribution path. This is still a strategic decision that should be evaluated as an issue first.
There was a problem hiding this comment.
Ok, I'm done for the day but you should open an issue about this. I agree that GitHub can become the new distribution path. The tar file method is already dead I think, since it's manually triggered and 0.198 is the last version that will be produced that way. You can still produce 0.198 from github sources, but 0.200 will use an as-yet-unwritten tar building script running in CI.
There was a problem hiding this comment.
I suggest to merge this - there was no real objection #19 after more than a week.
metamath.c
Outdated
| @@ -1138,7 +1138,7 @@ void command(int argc, char *argv[]) | |||
| } | |||
| } | |||
| /* Do the operating system command */ | |||
| (void)system(str1); | |||
| (void)!system(str1); | |||
There was a problem hiding this comment.
If we want a coding style, perhaps we could get ambitious and shoot for "don't ignore the return value from system" (because it could fail if out of memory, out of file descriptors, etc)?
I mean, we have gotten by with this code for a long time, so I'm not saying this is necessarily our top priority, just that.... well putting in a simple error check might not kill us.
There was only one use of
ssize_tand it wasn't needed, so I've just removed it. It seems to give errors at-std=c99for some compilers, including mine (gcc 9.3.0).