On 9/23/21 11:03 AM, Ævar Arnfjörð Bjarmason wrote:
On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:
+ switch (sbgr) {
+ case SBGR_READY:
+ return 0;
- else if (pid_seen == pid_child) {
- /*
- * The new child daemon process shutdown while
- * it was starting up, so it is not listening
- * on the socket.
- *
- * Try to ping the socket in the odd chance
- * that another daemon started (or was already
- * running) while our child was starting.
- *
- * Again, we don't care who services the socket.
- */
- s = ipc_get_active_state(cl_args.path);
- if (s == IPC_STATE__LISTENING)
- return 0;
+ default:
+ case SBGR_ERROR:
+ case SBGR_CB_ERROR:
+ return error("daemon failed to start");
There was a discussion on v1 about the "default" being redundant here
and hiding future compiler checks, this is another "not sure what you
thought of that" case (per [1]).
Interestingly in this case if I drop the "default" my local gcc
uncharacteristically complains about a missing "return" in this
function, but clang doesn't. I needed to add a BUG() to shut up the
former. Maybe I'm wrong, but perhaps it's a sign of some deeper
trouble. This is with gcc/clang 10.2.1-6/11.0.1-2.
The issue of whether C needs a "default" case in switch statements
on an enum is one I didn't have bandwidth to think about (and is
completely independent of my series).
I was thinking that as a later task, someone could investigate which
compilers do and do not generate errors for missing enum values in
the switch. Perhaps that leads to a macro in config.mak.uname on
a system-by-system basis that "does the right thing".
Then one could have something like:
switch (e) {
DEFAULT_HANDLER;
case e1: ...
case e2: ...
}
1. https://lore.kernel.org/git/87v92r49mt.fsf@xxxxxxxxxxxxxxxxxxx/
I played with the diff below on top of this, I can't remember if it was
noted already, but the way you declare function ptrs and use them isn't
the usual style:
-- >8 --
diff --git a/run-command.c b/run-command.c
index 76bbef9d96d..5c831545201 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
}
enum start_bg_result start_bg_command(struct child_process *cmd,
- start_bg_wait_cb *wait_cb,
+ start_bg_wait_cb wait_cb,
void *cb_data,
unsigned int timeout_sec)
{
diff --git a/run-command.h b/run-command.h
index 17b1b80c3d7..e8a637d1967 100644
--- a/run-command.h
+++ b/run-command.h
@@ -527,7 +527,7 @@ enum start_bg_result {
* Returns 0 if child is "ready".
* Returns -1 on any error and cause start_bg_command() to also error out.
*/
-typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
+typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
By defining the typedef WITHOUT the star, we get a function type.
We can then use it for forward function declarations.
But additionally, when declare a function that takes a function
pointer or when we define a vtable of function pointers, they look
like pointers.
start_bg_wait_cb *pfn = my_cb;
int foo(struct child_process *cmd, start_bg_wait_cb *cb);
Or if we look a the target vtable in Trace2. This looks like
a structure of (function) pointers.
struct tr2_tgt {
tr2_tgt_init_t *pfn_init;
tr2_tgt_term_t *pfn_term;
...
};
So I prefer to leave the star out of function typedef and then
we can use the typedef in both contexts.
Jeff
