All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Hostetler <git@jeffhostetler.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>,
	Taylor Blau <me@ttaylorr.com>, Carlo Arenas <carenas@gmail.com>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v2 0/7] Builtin FSMonitor Part 1
Date: Thu, 23 Sep 2021 13:12:49 -0400	[thread overview]
Message-ID: <0ec69aff-40a9-aac1-5fca-08033c967d88@jeffhostetler.com> (raw)
In-Reply-To: <87v92r49mt.fsf@evledraar.gmail.com>



On 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> Here is V2 of Part 1 of my Builtin FSMonitor series.
>>
>> Changes since V1 include:
>>
>>   * Drop the Trace2 memory leak.
>>   * Added a new "child_ready" event to Trace2 as an alternative to the
>>     "child_exit" event for background processes.
>>   * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use
>>     the new "child_ready" event.
>>   * Various minor code and documentation cleanups.
> 
> I see 7/7 still has a pattern you included only to make a compiler error
> better. I noted in
> https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it
> make the error worse, on at least clang. You didn't note which compiler
> you were massaging, presumably MSVC.
> 

I've been holding my tongue for days on this issue and hoping a third
party would step in an render an opinion one way or another.

Too me, a forward declaration seemed like no big deal and it does
have value as I tried to explain.  And frankly, it felt a little bit
like bike-shedding and was trying to avoid that again.


The error message I quoted was from Clang v11.0.3.  My forward
declaration of the function prior to the actual definition of
the function causes the compiler to stop at the function definition
and complain with a short message saying that the function itself
is incorrectly defined and doesn't match the typedef that it is
associated with.

When I use MSVC I get a similar error at the function definition.

When I use GCC I get error messages at both the function definition
and the usage in the call.


Additionally, the forward declaration states that the function is
associated with that typedef (something that is otherwise implicit
and may be hard to discover (more on that in a minute)).

And it doesn't require a reference to the function pointer (either
on the right side of an assignment, a vtable initialization, or passing
it in a function call) to flag the error.  We always get the error
at the point of the definition.


The error message in your example is, I feel, worse than mine.
It splats 2 different function signatures -- only one of which has
the typedef name -- in a large, poorly wrapped brick of text.

Yes, your error message does print corresponding arg in the function
prototype of "start_bg_command()" that doesn't agree with the symbol
used at the call-site, but that is much later than where the actual
error occurred.  And if the forward declaration were present, you'd
already know that back up at the definition, right.


Let's look at this from another point of view.

Suppose for example we have two function prototypes with the same
signature.  Perhaps they describe groups of functions with different
semantics -- the fact that they have the same argument list and return
type is just a coincidence.

     typedef int(t_fn_1)(int);
     typedef int(t_fn_2)(int);

And then declare one or more instances of functions in those groups:

     int foo_a(int x) { ... }
     int foo_b(int x) { ... }
     int foo_c(int x) { ... }
     int foo_d(int x) { ... }
     int foo_e(int x) { ... }
     int foo_f(int x) { ... }
     int foo_g(int x) { ... }

Which of those functions should be associated with "t_fn_1" and which
with "t_fn_2"?   Again, they all have the same signature, but different
semantics.  The author knows when they wrote the code, but it may be
hard to automatically determine later.

If I then have a function like start_bg_command() that receives a
function pointer:

     int test(..., t_fn_1 *fn, ...) { ... }

In C -- even with the use of forward function declarations -- the
compiler won't complain if you pass test() a pointer of type t_fn_2
-- as long a t_fn_1 and t_fn_2 have the same signature.

But it does give the human a chance to catch the error.  Of if we
later change the function signature in the t_fn_1 typedef, we will
automatically get a list of which of those foo_x functions do and
do not need to be updated.


Anyway, I've soapboxed on this enough.  I think it is a worthy
feature for the price.


Jeff

  reply	other threads:[~2021-09-23 17:13 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 20:36 [PATCH 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-15 20:36 ` [PATCH 1/7] trace2: fix memory leak of thread name Jeff Hostetler via GitGitGadget
2021-09-15 21:01   ` Junio C Hamano
2021-09-16  4:19     ` Taylor Blau
2021-09-16  5:35   ` Ævar Arnfjörð Bjarmason
2021-09-16  5:43     ` Taylor Blau
2021-09-16  8:01       ` Ævar Arnfjörð Bjarmason
2021-09-16 15:35         ` Jeff Hostetler
2021-09-16 15:47           ` Ævar Arnfjörð Bjarmason
2021-09-16 19:13         ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-15 20:43   ` Eric Sunshine
2021-09-17 16:52     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-15 21:06   ` Junio C Hamano
2021-09-17 16:58     ` Jeff Hostetler
2021-09-18  7:03       ` Carlo Arenas
2021-09-20 15:51       ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-16  5:40   ` Ævar Arnfjörð Bjarmason
2021-09-17 17:27     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-16  5:47   ` Ævar Arnfjörð Bjarmason
2021-09-17 18:10     ` Jeff Hostetler
2021-09-17 19:14       ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  4:53   ` Taylor Blau
2021-09-16  4:58     ` Taylor Blau
2021-09-16  5:56   ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  5:06   ` Taylor Blau
2021-09-17 19:41     ` Jeff Hostetler
2021-09-18  8:59       ` Ævar Arnfjörð Bjarmason
2021-09-16  5:55   ` Ævar Arnfjörð Bjarmason
2021-09-20 15:36 ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-23 15:03     ` Ævar Arnfjörð Bjarmason
2021-09-23 17:58       ` Jeff Hostetler
2021-09-23 18:37       ` Junio C Hamano
2021-11-04 19:46     ` Adam Dinwoodie
2021-11-04 20:14       ` Ramsay Jones
2021-11-08 14:58       ` Jeff Hostetler
2021-11-08 23:59       ` Johannes Schindelin
2021-11-09 18:53         ` Ramsay Jones
2021-11-09 23:01           ` Johannes Schindelin
2021-11-09 23:34             ` Junio C Hamano
2021-11-10 12:27               ` Johannes Schindelin
2021-11-12  8:56                 ` Adam Dinwoodie
2021-11-12 16:01                   ` Junio C Hamano
2021-11-12 21:33                     ` Adam Dinwoodie
2021-11-16 10:56                       ` Johannes Schindelin
2021-11-16 11:02                   ` Johannes Schindelin
2021-11-13 19:11                 ` Ramsay Jones
2021-11-14 19:34                   ` Ramsay Jones
2021-11-14 20:10                   ` Adam Dinwoodie
2021-09-23 14:33   ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Ævar Arnfjörð Bjarmason
2021-09-23 17:12     ` Jeff Hostetler [this message]
2021-09-23 20:47       ` Ævar Arnfjörð Bjarmason
2021-09-27 13:37         ` Jeff Hostetler
2021-09-23 17:51     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0ec69aff-40a9-aac1-5fca-08033c967d88@jeffhostetler.com \
    --to=git@jeffhostetler.com \
    --cc=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    --cc=me@ttaylorr.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.