davmac314 / dinit

Service monitoring / "init" system
Apache License 2.0
620 stars 49 forks source link

Draft: Misc gcc fixes #408

Closed WavyEbuilder closed 3 weeks ago

WavyEbuilder commented 3 weeks ago

Marked as draft for now as there are probably some things that need discussing first. Fixes a few warnings when building with GCC.

For this warning:

[27/31] Compiling C++ object src/dinit.p/control.cc.o
../src/control.cc: In member function ‘bool control_conn_t::send_data()’:
../src/control.cc:1663:26: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1663 |             bp_sys::write(iob.get_watched_fd(), oomBuf, 1);
      |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm guessing the result of bp_sys::write is being ignored as we are at an oom state anyway? I wanted to confirm that before adding a comment, and if that is a case, should I add an explanatory comment explaining why we are throwing away the result of the function?

davmac314 commented 3 weeks ago

I'm guessing the result of bp_sys::write is being ignored as we are at an oom state anyway?

It's ignored because the connection will be closed regardless (yes, due to OOM) and there's no meaningful way to handle an error at this point.

In some other cases return values from system calls are ignored because they are used in contexts where they can't realistically fail.

davmac314 commented 3 weeks ago

This is so far just removing false positives. If the warnings are bothering you, just disable them when compiling. Sorry, but I don't really want the churn of adding a load of workarounds to silence warnings which are different from compiler to compiler (and version to version) anyway. I would prefer if you just close this unless there are genuine errors that need fixing.

By all means add (void) casts in any new code, but it's not worth going through and adding lots of them now.

WavyEbuilder commented 3 weeks ago

Makes sense, understandable to not add the baggage. I'll close this based on this, thanks for the feedback. :)