On Thu, Sep 23 2021, Jeff Hostetler wrote: > 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@xxxxxxxxxxxxxxxxxxx/ 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. I agree with you that it's no big deal in the end, I thought I made it clear in <87v92r49mt.fsf@xxxxxxxxxxxxxxxxxxx> but the main thing I'm commenting on is not that I or anyone else suggested Y over X, and you said nah and went for X in the end. That's fine, I mean, depending on the comment/issue etc. it's something other reviewers & Junio can draw their own conclusions about. What I am saying that it's much better for review of iterations of patches in general, and especially of a complex multi-part series if reviewers don't have to read the cover letter of vX and wonder what's omitted/unaddressed in the V(X-1) comments, and then go and re-read the discussion themselves. It's not the "nah", but that the "nah" is implicit and only apparent when sending an E-Mail like this. Of course that's never perfect, you can't summarize every point etc. Personally I try to do this, but I've sometimes noticed after the fact that I've gotten it wrong etc. In the end I and I think anyone else offering their time to review things is trying to move the relevant topic forward in one way or another. I'd much rather spend my time on a vX discussing new things & getting the thing closer to merge-able state, than re-reading all of v(X-1) & effectively coming up with my own cover letter summary in my head or in my own notes as I read along. Anyway, sorry about the bikeshedding getting out of hand, and what seems to have been at least partially a misunderstanding in the last couple of E-Mails between us, but the above is all I was going for. > 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. For what it's worth any poor wrapping is my fault, I have a relatively wide terminal and re-wrapped this when composing the E-Mail. I think both GCC & Clang (and most other mature compilers) would give the person getting the error sane wrapping based on their $COLUMNS. > 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. Code in git.git generally just declares say an "int foo(int)" and leaves it at passing the "foo", we're not concerned about that "foo" just so happening to be passed to some other interface that takes the same signature, certainly not something within a <1k line t/helper/* file. We all have habits we've picked up from other codebases prior to working on git.git. I'm not arguing that what you're describing is worse in some abtract sense, but that there's a larger value in following conventions within the codebase as Junio noted in his reply.
