Booleans

Booleans

[Eli Zaretskii]

Paul, I'm not sure I understand why we want such a wholesale
replacement of 0/1 with false/true.  It's okay to make such changes as
part of other development, same as we do with whitespace, but why is
it a good idea to convert everything, including bitfields (which
required introduction of a new non-standard typedef), into booleans?

In any case, this part:

  -   If CCL_PROG is nil, we just reset the structure pointed by CCL.  */
  -int
  +   If CCL_PROG is nil, just reset the structure pointed by CCL.  */
  +bool
   setup_ccl_program (struct ccl_program *ccl, Lisp_Object ccl_prog)
   {
     int i;
  @@ -1933,7 +1931,7 @@ setup_ccl_program (struct ccl_program *c

	 ccl_prog = ccl_get_compiled_code (ccl_prog, &ccl->idx);
	 if (! VECTORP (ccl_prog))
  -       return -1;
  +       return false;
	 vp = XVECTOR (ccl_prog);
	 ccl->size = vp->header.size;
	 ccl->prog = vp->contents;
  @@ -1950,14 +1948,11 @@ setup_ccl_program (struct ccl_program *c
     ccl->ic = CCL_HEADER_MAIN;
     for (i = 0; i < 8; i++)
       ccl->reg[i] = 0;
  -  ccl->last_block = 0;
  -  ccl->private_state = 0;
  +  ccl->last_block = false;
     ccl->status = 0;
     ccl->stack_idx = 0;
  -  ccl->suppress_error = 0;
  -  ccl->eight_bit_control = 0;
  -  ccl->quit_silently = 0;
  -  return 0;
  +  ccl->quit_silently = false;
  +  return true;
   }

as well as related changes, IMO should have been a separate commit, as
it is not a mechanical replacement, far from it.

And if changes like this:

  -#if 0
  +#if false

are intentional, then I'm afraid I don't see the point; please
explain.

Thanks.

Re: Booleans

[Paul Eggert]

Eli Zaretskii wrote:

> I'm not sure I understand why we want such a wholesale
> replacement of 0/1 with false/true.

It's no big deal and we could easily change it back if
desired.  I did it primarily because Stefan suggested that I
do so, while changing int to bool etc.

> why is it a good idea to convert everything, including bitfields (which
> required introduction of a new non-standard typedef), into booleans?

If a value is a boolean, it typically makes things easer on
the reader, the debugger, etc. to know that it's a boolean
and not an integer, so that one needn't worry about values
other than 0 and 1.

bool_bf is a response to trunk bzr 115272, which used 'bool
inhibit_shrinking : 1;'.  This is not portable to pre-C99
platforms.  I temporarily fixed that by changing 'bool' by
'unsigned' in trunk bzr 115280, but for maintainability
'bool' is indeed better, and 'bool_bf' attempts to support
this while still maintaining portability to pre-C99
platforms.

If we could assume C99 or later, we could get rid of bool_bf
and just use bool.  I'm thinking of proposing such an
assumption for after the next release.  This would allow us
to use some other C99 features, such as declarations after
statements, which would make Emacs easier to maintain.

> should have been a separate commit, as
> it is not a mechanical replacement, far from it.

It was a judgment call.  Perhaps I should have separated it
out, though it's no big deal.  It was a case of an int value
being used as a boolean with values 0 and -1.

>   -#if 0
>   +#if false
> 
> ... I'm afraid I don't see the point

I tried to be reasonably systematic in replacing uses of 0
and 1 as booleans in the set of files that I was looking at;
this included the use of booleans in preprocessor
expressions.  It would be an odd style to use '0' for false
preprocessor expressions, while using 'false' for false
expressions everywhere else.

Re: Booleans

[Stefan Monnier]

> It's no big deal and we could easily change it back if
> desired.  I did it primarily because Stefan suggested that I
> do so, while changing int to bool etc.

But indeed, it's better to do it only where you already make other changes.
If you remember, I mentioned it on a line you changed from

    int foo = 0;
to
    bool foo = 0;


-- Stefan

Re: Booleans

[Eli Zaretskii]

> Date: Sun, 15 Dec 2013 15:03:50 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> > should have been a separate commit, as
> > it is not a mechanical replacement, far from it.
> 
> It was a judgment call.  Perhaps I should have separated it
> out, though it's no big deal.  It was a case of an int value
> being used as a boolean with values 0 and -1.

Yes, but you also removed several struct members in that hunk.

> >   -#if 0
> >   +#if false
> > 
> > ... I'm afraid I don't see the point
> 
> I tried to be reasonably systematic in replacing uses of 0
> and 1 as booleans in the set of files that I was looking at;
> this included the use of booleans in preprocessor
> expressions.  It would be an odd style to use '0' for false
> preprocessor expressions, while using 'false' for false
> expressions everywhere else.

"#if false" looks very odd to me, I don't think I've ever seen that.

Re: Booleans

[Jarek Czekalski]

W dniu 2013-12-16 00:03, Paul Eggert pisze:
> I did it primarily because Stefan suggested that I
> do so, while changing int to bool etc.

Was it a private discussion? I can't remember seeing that. I hope it was 
not, see "Avoid private discussions". [1]

>> It would be an odd style to use '0' for false
>> preprocessor expressions, while using 'false' for false
>> expressions everywhere else.

For a c programmer while (1) is extremely readable and obvious, and 
short. While we are in c and not in Python or Lisp, I would follow the c 
convention. In places like this one, where introducing bool gives no 
benefits.

> >should have been a separate commit, as
> >it is not a mechanical replacement, far from it.
> It was a judgment call.  Perhaps I should have separated it
> out, though it's no big deal.  It was a case of an int value
> being used as a boolean with values 0 and -1.

I think it is a deal worth a while. It's very hard to locate a change in 
a commit that is 221k long. Such long commits should be limited to 
mechanical changes. Putting several different changes in such a long 
commit makes it impossible to review. Now it's time for the most 
important question: do you want your revisions to be reviewed? See 
"Practice Conspicuous Code Review" [2].

Why this single change should be a separate commit? I would justify it 
being a change of the interface of an internal library. An advanced 
Emacs user may have more calls to this internal api. Would he expect 
that the interface was changed seeing a label "Use bool for boolean" in 
the commit message? I could even give reasons for not changing at all 
this interface, but maybe it's not worth arguing.

Thanks
Jarek

[1] http://producingoss.com/en/producingoss.html#avoid-private-discussions
[2] http://producingoss.com/en/producingoss.html#code-review

Re: Booleans

[Paul Eggert]

Stefan Monnier wrote:

> it's better to do it only where you already make other changes.

Ah, I interpreted that to mean "while you're in the same file",
but I guess you meant "while you're in the same line",
or something like that.  It seemed odd to change a
declaration from 'int foo = 0;' to 'bool foo = 0;', while
leaving a later 'foo = 1' undisturbed because it's far
from the declaration, which is why I took a more expansive
interpretation.

Here's the state of the 'bool' pass so far.  *.h files now
use 'bool', 'true', and 'false' for boolean, because of the
most-recent big patch.  .c files from src/alloc.c through
src/undo.c now use 'bool' but not 'true' and 'false',
because of earlier patches.  The remaining .c files largely
have yet to be looked at systematically; I suppose I might
change them to use 'bool', and to use 'true' and 'false' if
connected to a 'bool' change, while leaving constructions
like 'while (0)' and '#if 0' alone.

Obviously none of this is urgent.

Eli Zaretskii wrote:

> you also removed several struct members in that hunk.

They were mostly unused int members that were probably
intended to be bool.  Rather than try to guess types
it was simpler to remove unused members.  This part of
Emacs is so little used that it doesn't really matter much.

> "#if false" looks very odd to me, I don't think I've ever seen that.

It won't appear in any code intended to be portable to
pre-C99 (at least, not without something like gnulib), which
is why it looks "funny" to developers whose experience is
based on such programs.  If people prefer "#if 0" that's fine.

Jarek Czekalski wrote:

> Was it a private discussion?

Yes.  In relatively-unimportant matters, communicating
privately often saves everybody time.  One can't always
judge in advance which cases these will be, alas.

> Why this single change should be a separate commit?

In the past, I've been asked to not use lots of little
commits for related changes; I've also been asked to not
have one big commit when changes can be separated out.  So I
try to use my best judgment as to which method to use, and
(as we've seen here) my judgment is fallible.