Warning message in remote.c when compiling

Warning message in remote.c when compiling

[prpr 19xx]

Hi,

I get this warning message when compiling remote.c:

...
    CC remote.o
remote.c:596: warning: 'remotes_remote_get' declared inline after being called
remote.c:596: warning: previous declaration of 'remotes_remote_get' was here
    CC replace-object.o
...

This is from the "master" branch, but it's the same on "next". It's
easily fixed with this patch:

diff --git a/remote.c b/remote.c
index 2b650b8..347f504 100644
--- a/remote.c
+++ b/remote.c
@@ -592,7 +592,7 @@ const char *pushremote_for_branch(struct branch
*branch, int *explicit)
                                             branch, explicit);
 }

-static struct remote *remotes_remote_get(struct remote_state *remote_state,
+static inline struct remote *remotes_remote_get(struct remote_state
*remote_state,
                                         const char *name);

 const char *remote_ref_for_branch(struct branch *branch, int for_push)

Thanks.

Re: Warning message in remote.c when compiling

[Dragan Simic]

Hello,

On 2024-04-06 16:21, prpr 19xx wrote:
> I get this warning message when compiling remote.c:
> 
> ...
>     CC remote.o
> remote.c:596: warning: 'remotes_remote_get' declared inline after being 
> called
> remote.c:596: warning: previous declaration of 'remotes_remote_get' was 
> here
>     CC replace-object.o
> ...

Could you, please, provide more details about your environment,
i.e. the operating system and compiler?

> This is from the "master" branch, but it's the same on "next". It's
> easily fixed with this patch:
> 
> diff --git a/remote.c b/remote.c
> index 2b650b8..347f504 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -592,7 +592,7 @@ const char *pushremote_for_branch(struct branch
> *branch, int *explicit)
>                                              branch, explicit);
>  }
> 
> -static struct remote *remotes_remote_get(struct remote_state 
> *remote_state,
> +static inline struct remote *remotes_remote_get(struct remote_state
> *remote_state,
>                                          const char *name);
> 
>  const char *remote_ref_for_branch(struct branch *branch, int for_push)
> 
> Thanks.

Re: Warning message in remote.c when compiling

[Jeff King]

On Sat, Apr 06, 2024 at 06:12:34PM +0200, Dragan Simic wrote:

> Hello,
> 
> On 2024-04-06 16:21, prpr 19xx wrote:
> > I get this warning message when compiling remote.c:
> > 
> > ...
> >     CC remote.o
> > remote.c:596: warning: 'remotes_remote_get' declared inline after being
> > called
> > remote.c:596: warning: previous declaration of 'remotes_remote_get' was
> > here
> >     CC replace-object.o
> > ...
> 
> Could you, please, provide more details about your environment,
> i.e. the operating system and compiler?

I'm also curious about which compiler, but I think it's a reasonable
complaint. We forward-declare the static function, use it, and then
later declare it inline. I didn't check to see what the standard says,
but it seems like a funny thing to do in general.

It has been that way for a while; since 56eed3422c (remote: remove
the_repository->remote_state from static methods, 2021-11-17), I think.

I don't really see any need to mark the wrapper as inline. It's one
basic function call (on top of an interface which requires a callback
anyway!), and I suspect many compilers would consider inlining anyway,
since it's a static function.

Ditto for remotes_pushremote_get(), though it doesn't have a forward
declaration.

-Peff

Re: Warning message in remote.c when compiling

[Dragan Simic]

Hello Jeff,

On 2024-04-07 03:38, Jeff King wrote:
> On Sat, Apr 06, 2024 at 06:12:34PM +0200, Dragan Simic wrote:
> 
>> Hello,
>> 
>> On 2024-04-06 16:21, prpr 19xx wrote:
>> > I get this warning message when compiling remote.c:
>> >
>> > ...
>> >     CC remote.o
>> > remote.c:596: warning: 'remotes_remote_get' declared inline after being
>> > called
>> > remote.c:596: warning: previous declaration of 'remotes_remote_get' was
>> > here
>> >     CC replace-object.o
>> > ...
>> 
>> Could you, please, provide more details about your environment,
>> i.e. the operating system and compiler?
> 
> I'm also curious about which compiler, but I think it's a reasonable
> complaint. We forward-declare the static function, use it, and then
> later declare it inline. I didn't check to see what the standard says,
> but it seems like a funny thing to do in general.

The link below seems to provide more details.  The way I see it,
declarations and definitions should match, and the standard seems
to support that.  Though, not all compilers (or not all versions)
complain in this particular case.

https://stackoverflow.com/a/62390378/22330192

> It has been that way for a while; since 56eed3422c (remote: remove
> the_repository->remote_state from static methods, 2021-11-17), I think.
> 
> I don't really see any need to mark the wrapper as inline. It's one
> basic function call (on top of an interface which requires a callback
> anyway!), and I suspect many compilers would consider inlining anyway,
> since it's a static function.
> 
> Ditto for remotes_pushremote_get(), though it doesn't have a forward
> declaration.
> 
> -Peff

Re: Warning message in remote.c when compiling

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't really see any need to mark the wrapper as inline. It's one
> basic function call (on top of an interface which requires a callback
> anyway!), and I suspect many compilers would consider inlining anyway,
> since it's a static function.
>
> Ditto for remotes_pushremote_get(), though it doesn't have a forward
> declaration.

Yup.  I presonally feel that we should get rid of "static inline"
unless they appear in header files.  The compilers should in general
be able to do good enough job finding what to inline than we can (1)
initially mark what to inline, and (2) update by dropping "inline"
that is no longer appropriate as the code evolves.

Thanks.

Re: Warning message in remote.c when compiling

[prpr 19xx]

On Sun, 7 Apr 2024 at 06:40, Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > I don't really see any need to mark the wrapper as inline. It's one
> > basic function call (on top of an interface which requires a callback
> > anyway!), and I suspect many compilers would consider inlining anyway,
> > since it's a static function.
> >
> > Ditto for remotes_pushremote_get(), though it doesn't have a forward
> > declaration.
>
> Yup.  I presonally feel that we should get rid of "static inline"
> unless they appear in header files.  The compilers should in general
> be able to do good enough job finding what to inline than we can (1)
> initially mark what to inline, and (2) update by dropping "inline"
> that is no longer appropriate as the code evolves.

The compiler is an ancient gcc 4.2.0 cross-compiler for a
mipsel-linux-uclibc environment.
It doesn't really matter though, as folk seem to agree the definition
and declaration should match, which I think they should.
I also agree that having inline probably makes no sense, as the
compiler can usually work this stuff out itself.
So I don't mind whether all the inlines get removed or they all stay,
as long as they are all effectively consistent, which they are
currently not, and the compiler righty (to my mind) complains.

Thanks.