| Message ID | CAA1Aqdvj6Eyp9jGaAxTf8p0Eh_rCPydOpin3D5QYHy8sqOoOsw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Warning message in remote.c when compiling | expand |
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.
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
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
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.
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.
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);
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: const char *remote_ref_for_branch(struct branch *branch, int for_push) Thanks.