Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Eli Zaretskii]

> --- a/src/doc.c	2012-09-02 16:56:31 +0000
> +++ b/src/doc.c	2012-09-03 09:22:43 +0000
> @@ -848,9 +848,10 @@
>  	  struct buffer *oldbuf;
>  	  ptrdiff_t start_idx;
>  	  /* This is for computing the SHADOWS arg for describe_map_tree.  */
> -	  Lisp_Object active_maps = Fcurrent_active_maps (Qnil, Qnil);
> +	  Lisp_Object active_maps;
>  	  Lisp_Object earlier_maps;
>  
> +	  active_maps = Fcurrent_active_maps (Qnil, Qnil);
>  	  changed = 1;
>  	  strp += 2;		/* skip \{ or \< */
>  	  start = strp;

What problems did static checking find in this (and other similar)
fragment?

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Stefan Monnier]

>> --- a/src/doc.c	2012-09-02 16:56:31 +0000
>> +++ b/src/doc.c	2012-09-03 09:22:43 +0000
>> @@ -848,9 +848,10 @@
>> struct buffer *oldbuf;
>> ptrdiff_t start_idx;
>> /* This is for computing the SHADOWS arg for describe_map_tree.  */
>> -	  Lisp_Object active_maps = Fcurrent_active_maps (Qnil, Qnil);
>> +	  Lisp_Object active_maps;
>> Lisp_Object earlier_maps;
>> 
>> +	  active_maps = Fcurrent_active_maps (Qnil, Qnil);
>> changed = 1;
>> strp += 2;		/* skip \{ or \< */
>> start = strp;

> What problems did static checking find in this (and other similar)
> fragment?

Yes, I curious, especially since the resulting code is less elegant.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Paul Eggert]

On 09/03/2012 08:35 AM, Eli Zaretskii wrote:
 
> What problems did static checking find in this (and other similar)
> fragment?

It didn't find any problems in those two cases, as near as I can
tell; it was a false alarm by the new GCC flag -Wjump-misses-init.

Since those fragments annoy, I reverted them and instead changed
configure.ac to not use that GCC flag.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Eli Zaretskii]

> Date: Mon, 03 Sep 2012 12:37:12 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> On 09/03/2012 08:35 AM, Eli Zaretskii wrote:
>  
> > What problems did static checking find in this (and other similar)
> > fragment?
> 
> It didn't find any problems in those two cases, as near as I can
> tell; it was a false alarm by the new GCC flag -Wjump-misses-init.

Thanks for the explanation.

> Since those fragments annoy

GCC false alarms aside, are there any reasons to prefer separate
declaration and initialization?

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Paul Eggert]

On 09/03/2012 01:44 PM, Eli Zaretskii wrote:
> GCC false alarms aside, are there any reasons to prefer separate
> declaration and initialization?

I suspect the warning is designed more to enforce the C99
rule that one cannot jump over a declaration of a local
identifier whose type is variably modified.  If Emacs ever
starts using those types we may want to look into this again.

Or we may try cutting back on those gotos -- they are kinda
confusing ....

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Stefan Monnier]

> GCC false alarms aside, are there any reasons to prefer separate
> declaration and initialization?

There are software-engineering reasons to prefer the "initialize as part
of declare" so it's trivially obvious that the variable can't
be uninitialized.  So in general, we'd first want to investigate other
solutions if such an "init within declare" causes problems.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Jan Djärv]

Hello.

4 sep 2012 kl. 05:41 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>> GCC false alarms aside, are there any reasons to prefer separate
>> declaration and initialization?
> 
> There are software-engineering reasons to prefer the "initialize as part
> of declare" so it's trivially obvious that the variable can't
> be uninitialized.  So in general, we'd first want to investigate other
> solutions if such an "init within declare" causes problems.

Another good thing is that init within declare variables can be declared const if they are not changed later.
This may not help compilers much nowdays, but it helps when reading code.

Emacs does not use const much. Is there a policy for its usage?

    Jan D.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Paul Eggert]

On 09/03/2012 10:29 PM, Jan Djärv wrote:
> Emacs does not use const much.

It's typically easy to determine whether a local variable
is never changed once initialized, simply by reading the code.
So, for local variables, 'const' is typically more trouble than
it's worth, as it adds a bit of noise to the code
without much compensating value.

For other things 'const' can be worthwhile sometimes,
as it can help the reader understand constraints on the code,
it can help avoid trivial programming errors,
and it can help the compiler generate better code.
Emacs tends to avoid 'const', though, partly
because it predates 'const, and partly I expect because
even in these other cases the readability advantage of
omitting 'const' arguably trumps the minor advantages
of using 'const'.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Jan Djärv]

Hello. 

4 sep 2012 kl. 07:45 skrev Paul Eggert <eggert@cs.ucla.edu>:

> On 09/03/2012 10:29 PM, Jan Djärv wrote:
>> Emacs does not use const much.
> 
> It's typically easy to determine whether a local variable
> is never changed once initialized, simply by reading the code.
> So, for local variables, 'const' is typically more trouble than
> it's worth, as it adds a bit of noise to the code
> without much compensating value.
> 

I disagree. Given that Emacs have quite a lot of functions that are more than a couple of hundred lines long, it is not typically easy.  For smaller functions, it can be, but small functions sometimes becomes big.

       Jan D.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Paul Eggert]

On 09/04/2012 01:31 AM, Jan Djärv wrote:

> Given that Emacs have quite a lot of functions that are more than a
> couple of hundred lines long, it is not typically easy.  For smaller
> functions, it can be, but small functions sometimes becomes big.

No doubt our styles differ, but I don't find it that hard.

For example, on my screen right now I have lib/mktime.c.  It has a
function __mktime_internal that is a couple of hundred lines long.  As
I understand it, you're proposing that almost all of its dozens of
local variables should be decorated with 'const'.  That is, something
like this:

  int min = tp->tm_min;
  int hour = tp->tm_hour;
  int mday = tp->tm_mday;
  int mon = tp->tm_mon;
  int year_requested = tp->tm_year;
  int isdst = tp->tm_isdst;

would be changed to something like this:

  const int min = tp->tm_min;
  const int hour = tp->tm_hour;
  const int mday = tp->tm_mday;
  const int mon = tp->tm_mon;
  const int year_requested = tp->tm_year;
  const int isdst = tp->tm_isdst;

I dunno.  To me those 'const's would take up valuable screen real
estate, and they would slow me down when I read the code, and it's
really not worth the trouble.  If I want to know whether mday is
modified later, I can just search for 'mday'; that's fast.  And in the
typical case where I'm not interested in that particular detail, the
'const's would be wasting my time.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Jan Djärv]

Hello.

4 sep 2012 kl. 10:58 skrev Paul Eggert <eggert@cs.ucla.edu>:

> On 09/04/2012 01:31 AM, Jan Djärv wrote:
> 
>> Given that Emacs have quite a lot of functions that are more than a
>> couple of hundred lines long, it is not typically easy.  For smaller
>> functions, it can be, but small functions sometimes becomes big.
> 
> No doubt our styles differ, but I don't find it that hard.
> 
> For example, on my screen right now I have lib/mktime.c.  It has a
> function __mktime_internal that is a couple of hundred lines long.  As
> I understand it, you're proposing that almost all of its dozens of
> local variables should be decorated with 'const'.  That is, something
> like this:
> 
>  int min = tp->tm_min;
>  int hour = tp->tm_hour;
>  int mday = tp->tm_mday;
>  int mon = tp->tm_mon;
>  int year_requested = tp->tm_year;
>  int isdst = tp->tm_isdst;
> 
> would be changed to something like this:
> 
>  const int min = tp->tm_min;
>  const int hour = tp->tm_hour;
>  const int mday = tp->tm_mday;
>  const int mon = tp->tm_mon;
>  const int year_requested = tp->tm_year;
>  const int isdst = tp->tm_isdst;
> 
> I dunno.  To me those 'const's would take up valuable screen real
> estate, and they would slow me down when I read the code, and it's
> really not worth the trouble.

Unless you have very narrow frames, this does not take up more screen estate, just whitespace estate.

If some variable is not const and then a pointer to it is passed into another function, you have to lookup that also. Or find the documentation if the source is not close by.

>  If I want to know whether mday is
> modified later, I can just search for 'mday'; that's fast.  And in the
> typical case where I'm not interested in that particular detail, the
> 'const's would be wasting my time.

I read "const X" as fast as "X". I mostly program in C++, I guess const is more used there.

    Jan D.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Jason Rumney]

Eli Zaretskii <eliz@gnu.org> writes:

>> --- a/src/doc.c	2012-09-02 16:56:31 +0000
>> +++ b/src/doc.c	2012-09-03 09:22:43 +0000
>> @@ -848,9 +848,10 @@
>>  	  struct buffer *oldbuf;
>>  	  ptrdiff_t start_idx;
>>  	  /* This is for computing the SHADOWS arg for describe_map_tree.  */
>> -	  Lisp_Object active_maps = Fcurrent_active_maps (Qnil, Qnil);
>> +	  Lisp_Object active_maps;
>>  	  Lisp_Object earlier_maps;
>>  
>> +	  active_maps = Fcurrent_active_maps (Qnil, Qnil);
>>  	  changed = 1;
>>  	  strp += 2;		/* skip \{ or \< */
>>  	  start = strp;
>
> What problems did static checking find in this (and other similar)
> fragment?

It's not valid C89 is it?  You can't declare variables after making a
function call within the same scope before C99, though few compilers
would trip on this with default compilation flags.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Jason Rumney]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Emacs tends to avoid 'const', though, partly
> because it predates 'const, and partly I expect because
> even in these other cases the readability advantage of
> omitting 'const' arguably trumps the minor advantages
> of using 'const'.

I think mostly because retrofitting const to a codebase the size of
Emacs is a huge task, and isn't something that can be done gradually.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Andreas Schwab]

Jason Rumney <jasonr@gnu.org> writes:

> It's not valid C89 is it?

Sure it is.  Declarations with initializer are allowed anywhere at the
start of the block.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Tue, 04 Sep 2012 16:33:54 +0200
> 
> Jason Rumney <jasonr@gnu.org> writes:
> 
> > It's not valid C89 is it?
> 
> Sure it is.  Declarations with initializer are allowed anywhere at the
> start of the block.

Yes.  But the fragment I cited didn't show the block beginning, which
is probably why Jason thought it's not valid C89.  Here's more
context (from another hunk of the same commit):

  --- a/src/xdisp.c	2012-09-02 17:10:35 +0000
  +++ b/src/xdisp.c	2012-09-03 09:22:43 +0000
  @@ -13511,9 +13511,10 @@
       }
     else if (FRAME_VISIBLE_P (sf) && !FRAME_OBSCURED_P (sf))
       {
  -      Lisp_Object mini_window = FRAME_MINIBUF_WINDOW (sf);
  +      Lisp_Object mini_window;
	 struct frame *mini_frame;

  +      mini_window = FRAME_MINIBUF_WINDOW (sf);
	 displayed_buffer = XBUFFER (XWINDOW (selected_window)->buffer);
	 /* Use list_of_error, not Qerror, so that
	   we catch only errors and don't run the debugger.  */

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Wojciech Meyer]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> GCC false alarms aside, are there any reasons to prefer separate
>> declaration and initialization?
>
> There are software-engineering reasons to prefer the "initialize as part
> of declare" so it's trivially obvious that the variable can't
> be uninitialized.  So in general, we'd first want to investigate other
> solutions if such an "init within declare" causes problems.

I would be also adding 'const' after such initialisation if not mutated
later.

>
>
>         Stefan
>
>

--
Wojciech Meyer
http://danmey.org

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Wojciech Meyer]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andreas Schwab <schwab@linux-m68k.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
>> Date: Tue, 04 Sep 2012 16:33:54 +0200
>>
>> Jason Rumney <jasonr@gnu.org> writes:
>>
>> > It's not valid C89 is it?
>>
>> Sure it is.  Declarations with initializer are allowed anywhere at the
>> start of the block.
>
> Yes.  But the fragment I cited didn't show the block beginning, which
> is probably why Jason thought it's not valid C89.  Here's more
> context (from another hunk of the same commit):
>
>   --- a/src/xdisp.c	2012-09-02 17:10:35 +0000
>   +++ b/src/xdisp.c	2012-09-03 09:22:43 +0000
>   @@ -13511,9 +13511,10 @@
>        }
>      else if (FRAME_VISIBLE_P (sf) && !FRAME_OBSCURED_P (sf))
>        {
>   -      Lisp_Object mini_window = FRAME_MINIBUF_WINDOW (sf);
>   +      Lisp_Object mini_window;
> 	 struct frame *mini_frame;
>
>   +      mini_window = FRAME_MINIBUF_WINDOW (sf);
> 	 displayed_buffer = XBUFFER (XWINDOW (selected_window)->buffer);
> 	 /* Use list_of_error, not Qerror, so that
> 	   we catch only errors and don't run the debugger.  */
>
>

Not initialising the variable makes sense only if we want to conform to
the archaic C standards, or we want later it to be modified, the gcc
compiler will know exactly when the variable has been used or modified
on some code path and will warn you accordingly.

--
Wojciech Meyer
http://danmey.org

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Wojciech Meyer]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 09/03/2012 10:29 PM, Jan Djärv wrote:
>> Emacs does not use const much.
>
> It's typically easy to determine whether a local variable
> is never changed once initialized, simply by reading the code.
> So, for local variables, 'const' is typically more trouble than
> it's worth, as it adds a bit of noise to the code
> without much compensating value.
>
> For other things 'const' can be worthwhile sometimes,
> as it can help the reader understand constraints on the code,
> it can help avoid trivial programming errors,
> and it can help the compiler generate better code.
> Emacs tends to avoid 'const', though, partly
> because it predates 'const, and partly I expect because
> even in these other cases the readability advantage of
> omitting 'const' arguably trumps the minor advantages
> of using 'const'.

+1.

Modifying the code with const is easier, if it's const then the rest of
the function can assume that it will not be changed, and the user will
be forced either to read in full the body and remove carefully the const
modifier, or just stick to the existing constraints.

--
Wojciech Meyer
http://danmey.org

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109864: Fix minor problems found by static checking.

[Stefan Monnier]

>> Emacs tends to avoid 'const', though, partly
>> because it predates 'const, and partly I expect because
>> even in these other cases the readability advantage of
>> omitting 'const' arguably trumps the minor advantages
>> of using 'const'.
> I think mostly because retrofitting const to a codebase the size of
> Emacs is a huge task, and isn't something that can be done gradually.

Indeed.  I don't mind people using `const' in their code, but please
don't go and add `const' all over the place.


        Stefan