all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Booleans
@ 2013-12-15 17:40 Eli Zaretskii
  2013-12-15 23:03 ` Booleans Paul Eggert
  0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2013-12-15 17:40 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Booleans
  2013-12-15 17:40 Booleans Eli Zaretskii
@ 2013-12-15 23:03 ` Paul Eggert
  2013-12-16  3:43   ` Booleans Stefan Monnier
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Paul Eggert @ 2013-12-15 23:03 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Booleans
  2013-12-15 23:03 ` Booleans Paul Eggert
@ 2013-12-16  3:43   ` Stefan Monnier
  2013-12-16  3:44   ` Booleans Eli Zaretskii
  2013-12-16  7:39   ` Booleans Jarek Czekalski
  2 siblings, 0 replies; 6+ messages in thread
From: Stefan Monnier @ 2013-12-16  3:43 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Eli Zaretskii, emacs-devel

> 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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Booleans
  2013-12-15 23:03 ` Booleans Paul Eggert
  2013-12-16  3:43   ` Booleans Stefan Monnier
@ 2013-12-16  3:44   ` Eli Zaretskii
  2013-12-16  7:39   ` Booleans Jarek Czekalski
  2 siblings, 0 replies; 6+ messages in thread
From: Eli Zaretskii @ 2013-12-16  3:44 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Booleans
  2013-12-15 23:03 ` Booleans Paul Eggert
  2013-12-16  3:43   ` Booleans Stefan Monnier
  2013-12-16  3:44   ` Booleans Eli Zaretskii
@ 2013-12-16  7:39   ` Jarek Czekalski
  2013-12-16 17:28     ` Booleans Paul Eggert
  2 siblings, 1 reply; 6+ messages in thread
From: Jarek Czekalski @ 2013-12-16  7:39 UTC (permalink / raw)
  To: emacs-devel


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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Booleans
  2013-12-16  7:39   ` Booleans Jarek Czekalski
@ 2013-12-16 17:28     ` Paul Eggert
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Eggert @ 2013-12-16 17:28 UTC (permalink / raw)
  To: emacs-devel

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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-12-16 17:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-15 17:40 Booleans Eli Zaretskii
2013-12-15 23:03 ` Booleans Paul Eggert
2013-12-16  3:43   ` Booleans Stefan Monnier
2013-12-16  3:44   ` Booleans Eli Zaretskii
2013-12-16  7:39   ` Booleans Jarek Czekalski
2013-12-16 17:28     ` Booleans Paul Eggert

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.