[PATCH] Remove warning about code not beeing executed
|
View:
New views
7 Messages
—
Rating Filter:
Alert me
|
 |
[PATCH] Remove warning about code not beeing executed
by Daniel Näslund
::
Rate this Message:
[[[
Fix warning of code never beeing executed. * subversion/libsvn_client/deprecated.c (wrap_pre_blame3_receiver): Add eol parameter to avoid check of strlen() against APR_EOL_STR. (svn_client_blame2, svn_client_blame): Update caller to pass APR_EOL_STR as eol. Patch by: Daniel Näslund <daniel@...> ]]] /Daniel [never_executed.diff] Index: subversion/libsvn_client/deprecated.c =================================================================== --- subversion/libsvn_client/deprecated.c (revision 40386) +++ subversion/libsvn_client/deprecated.c (arbetskopia) @@ -284,10 +284,10 @@ static void wrap_pre_blame3_receiver(svn_client_blame_receiver_t *receiver, - void **receiver_baton, + void **receiver_baton, const char *eol, apr_pool_t *pool) { - if (strlen(APR_EOL_STR) > 1) + if (strlen(eol) > 1) { struct wrapped_receiver_baton_s *b = apr_palloc(pool,sizeof(*b)); @@ -309,7 +309,7 @@ svn_client_ctx_t *ctx, apr_pool_t *pool) { - wrap_pre_blame3_receiver(&receiver, &receiver_baton, pool); + wrap_pre_blame3_receiver(&receiver, &receiver_baton, APR_EOL_STR, pool); return svn_client_blame3(target, peg_revision, start, end, svn_diff_file_options_create(pool), FALSE, receiver, receiver_baton, ctx, pool); @@ -323,7 +323,7 @@ svn_client_ctx_t *ctx, apr_pool_t *pool) { - wrap_pre_blame3_receiver(&receiver, &receiver_baton, pool); + wrap_pre_blame3_receiver(&receiver, &receiver_baton, APR_EOL_STR, pool); return svn_client_blame2(target, end, start, end, receiver, receiver_baton, ctx, pool); } |
|
|
Re: [PATCH] Remove warning about code not beeing executed
by Stefan Sperling-7
::
Rate this Message:
On Thu, Nov 05, 2009 at 07:28:18PM +0100, Daniel Näslund wrote:
> [[[ > Fix warning of code never beeing executed. > > * subversion/libsvn_client/deprecated.c > (wrap_pre_blame3_receiver): Add eol parameter to avoid check of > strlen() against APR_EOL_STR. > (svn_client_blame2, svn_client_blame): Update caller to pass > APR_EOL_STR as eol. > > Patch by: Daniel Näslund <daniel@...> > ]]] > > /Daniel > Index: subversion/libsvn_client/deprecated.c > =================================================================== > --- subversion/libsvn_client/deprecated.c (revision 40386) > +++ subversion/libsvn_client/deprecated.c (arbetskopia) > @@ -284,10 +284,10 @@ > > static void > wrap_pre_blame3_receiver(svn_client_blame_receiver_t *receiver, > - void **receiver_baton, > + void **receiver_baton, const char *eol, > apr_pool_t *pool) > { > - if (strlen(APR_EOL_STR) > 1) > + if (strlen(eol) > 1) No idea if we really need to fix this. There's a nice snippet in libsvn_subr/prompt.c which generates a similar warning but avoids strlen(): /* GCC might complain here: "warning: will never be executed" * That's fine. This is a compile-time check for "\r\n\0" */ if (sizeof(APR_EOL_STR) == 3) { saw_first_half_of_eol = TRUE; continue; } Stefan > { > struct wrapped_receiver_baton_s *b = apr_palloc(pool,sizeof(*b)); > > @@ -309,7 +309,7 @@ > svn_client_ctx_t *ctx, > apr_pool_t *pool) > { > - wrap_pre_blame3_receiver(&receiver, &receiver_baton, pool); > + wrap_pre_blame3_receiver(&receiver, &receiver_baton, APR_EOL_STR, pool); > return svn_client_blame3(target, peg_revision, start, end, > svn_diff_file_options_create(pool), FALSE, > receiver, receiver_baton, ctx, pool); > @@ -323,7 +323,7 @@ > svn_client_ctx_t *ctx, > apr_pool_t *pool) > { > - wrap_pre_blame3_receiver(&receiver, &receiver_baton, pool); > + wrap_pre_blame3_receiver(&receiver, &receiver_baton, APR_EOL_STR, pool); > return svn_client_blame2(target, end, start, end, > receiver, receiver_baton, ctx, pool); > } -- printf("Eh???/n"); ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414871 |
|
|
Re: [PATCH] Remove warning about code not beeing executed
by Julian Foad
::
Rate this Message:
Stefan Sperling wrote:
> On Thu, Nov 05, 2009 at 07:28:18PM +0100, Daniel Näslund wrote: > > static void > > wrap_pre_blame3_receiver(svn_client_blame_receiver_t *receiver, > > - void **receiver_baton, > > + void **receiver_baton, const char *eol, > > apr_pool_t *pool) > > { > > - if (strlen(APR_EOL_STR) > 1) > > + if (strlen(eol) > 1) > > No idea if we really need to fix this. The guiding principle is: never make code more complex without a VERY good reason. It's difficult enough as it is. Avoiding that warning is not, I think, a good enough reason to make that change. I think the ultimate analysis here is that the compiler doesn't know whether APR_EOL_STR ever has a different value. It warns because, in the current translation unit, that code will never be executed; but it doesn't know that sometimes we might ask it to compile with a different APR_EOL_STR. So, the problem here is that the warning is based on insufficient information. Maybe the next version of GCC will have a way to indicate that, or maybe it will stop issuing that warning in cases like this, or ... Anyway, if the warning is not very good, not very helpful, then we should disable it rather than twisting the code to avoid triggering it. After all, with your patch to pass the string as a parameter, a whole-file-optimiser could optimise out your transformation and still generate the same warning. - Julian > There's a nice snippet in libsvn_subr/prompt.c which generates a similar > warning but avoids strlen(): > > /* GCC might complain here: "warning: will never be executed" > * That's fine. This is a compile-time check for "\r\n\0" */ [...] ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414874 |
|
|
Re: [PATCH] Remove warning about code not beeing executed
by Daniel Shahaf-2
::
Rate this Message:
Julian Foad wrote on Thu, 5 Nov 2009 at 18:50 -0000:
> I think the ultimate analysis here is that the compiler doesn't know > whether APR_EOL_STR ever has a different value. It warns because, in the > current translation unit, that code will never be executed; but it > doesn't know that sometimes we might ask it to compile with a different > APR_EOL_STR Hmm. dannas mentioned yesterday that apr.h defines APR_EOL_STR unconditionally as "\n" (for him; as "\r\n" for me) --- i.e., not wrapped in any #if. Perhaps the warning would go away if apr.h chose between >=2 definitions for APR_EOL_STR? [[[ --- include/apr.h +++ include/apr.h @@ -N,M +N,M @@ +#ifdef WILL_NEVER_BE_DEFINED +#define APR_EOL_STR "\r\n\p\l\u\s\p\a\d\d\i\n\g" +#else #define APR_EOL_STR "\n" +#endif ]]] ? > So, the problem here is that the warning is based on > insufficient information. Maybe the next version of GCC will have a way > to indicate that, or maybe it will stop issuing that warning in cases > like this, or ... Anyway, if the warning is not very good, not very > helpful, then we should disable it rather than twisting the code to > avoid triggering it. After all, with your patch to pass the string as a > parameter, a whole-file-optimiser could optimise out your transformation > and still generate the same warning. ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414878 |
|
|
Re: [PATCH] Remove warning about code not beeing executed
by Julian Foad
::
Rate this Message:
On Thu, 2009-11-05 at 20:57 +0200, Daniel Shahaf wrote:
> Julian Foad wrote on Thu, 5 Nov 2009 at 18:50 -0000: > > I think the ultimate analysis here is that the compiler doesn't know > > whether APR_EOL_STR ever has a different value. It warns because, in the > > current translation unit, that code will never be executed; but it > > doesn't know that sometimes we might ask it to compile with a different > > APR_EOL_STR > > Hmm. dannas mentioned yesterday that apr.h defines APR_EOL_STR > unconditionally as "\n" (for him; as "\r\n" for me) --- i.e., not > wrapped in any #if. I think "apr.h" is generated by the APR build to match the platform it's built on. > Perhaps the warning would go away if apr.h chose between >=2 definitions > for APR_EOL_STR? > > [[[ > --- include/apr.h > +++ include/apr.h > @@ -N,M +N,M @@ > +#ifdef WILL_NEVER_BE_DEFINED > +#define APR_EOL_STR "\r\n\p\l\u\s\p\a\d\d\i\n\g" > +#else > #define APR_EOL_STR "\n" > +#endif > ]]] > > ? I don't think GCC would notice. > > So, the problem here is that the warning is based on > > insufficient information. Maybe the next version of GCC will have a way > > to indicate that, or maybe it will stop issuing that warning in cases > > like this, or ... Anyway, if the warning is not very good, not very > > helpful, then we should disable it rather than twisting the code to > > avoid triggering it. After all, with your patch to pass the string as a > > parameter, a whole-file-optimiser could optimise out your transformation > > and still generate the same warning. BTW, I am not saying any of these things are likely, just that they are theoretically possible. - Julian ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414889 |
|
|
Re: [PATCH] Remove warning about code not beeing executed
by Branko Cibej
::
Rate this Message:
Stefan Sperling wrote:
> On Thu, Nov 05, 2009 at 07:28:18PM +0100, Daniel Näslund wrote: > >> [[[ >> Fix warning of code never beeing executed. >> >> * subversion/libsvn_client/deprecated.c >> (wrap_pre_blame3_receiver): Add eol parameter to avoid check of >> strlen() against APR_EOL_STR. >> (svn_client_blame2, svn_client_blame): Update caller to pass >> APR_EOL_STR as eol. >> >> Patch by: Daniel Näslund <daniel@...> >> ]]] >> >> /Daniel >> > > >> Index: subversion/libsvn_client/deprecated.c >> =================================================================== >> --- subversion/libsvn_client/deprecated.c (revision 40386) >> +++ subversion/libsvn_client/deprecated.c (arbetskopia) >> @@ -284,10 +284,10 @@ >> >> static void >> wrap_pre_blame3_receiver(svn_client_blame_receiver_t *receiver, >> - void **receiver_baton, >> + void **receiver_baton, const char *eol, >> apr_pool_t *pool) >> { >> - if (strlen(APR_EOL_STR) > 1) >> + if (strlen(eol) > 1) >> > > No idea if we really need to fix this. > I think the result is way too convoluted to be considered a "fix". > There's a nice snippet in libsvn_subr/prompt.c which generates a similar > warning but avoids strlen(): > > /* GCC might complain here: "warning: will never be executed" > * That's fine. This is a compile-time check for "\r\n\0" */ > if (sizeof(APR_EOL_STR) == 3) > { > saw_first_half_of_eol = TRUE; > continue; > } > Yup. I'd recommend avoiding the strlen, but that's all. And *carefully* because there'a an off-by-one just waiting to happen when you replace strlen with sizeof. :) -- Brane ------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414923 |
|
|
Re: [PATCH] Remove warning about code not beeing executed
by Daniel Rall
::
Rate this Message:
r40395
------------------------------------------------------ http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414952 |
| Free embeddable forum powered by Nabble | Forum Help |
