bug#6493: 24.0.50; doc strings of region-beginning, region-end

bug#6493: 24.0.50; doc strings of region-beginning, region-end

[Drew Adams]

The doc strings just repeat the function names - they provide no helpful
info at all.
 
What we need to add is what is said in the Elisp manual: that the
"beginning" ("end") is the position of either point or the mark,
whichever is smaller (larger).

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2010-06-21 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4) --no-opt --cflags -Ic:/xpm/include'

bug#6493:

[Deniz Dogan]

[-- Attachment #1: Type: text/plain, Size: 38 bytes --]

How about this? (See attached patch.)

[-- Attachment #2: editfns-region-doc.patch --]
[-- Type: application/octet-stream, Size: 794 bytes --]

=== modified file 'src/editfns.c'
--- src/editfns.c	2010-06-16 20:08:41 +0000
+++ src/editfns.c	2010-06-27 12:22:00 +0000
@@ -349,14 +349,18 @@
 }
 
 DEFUN ("region-beginning", Fregion_beginning, Sregion_beginning, 0, 0, 0,
-       doc: /* Return position of beginning of region, as an integer.  */)
+       doc: /* Return position of beginning of region, as an integer.
+
+The beginning of the region is point or mark, whichever is smaller.  */)
      ()
 {
   return region_limit (1);
 }
 
 DEFUN ("region-end", Fregion_end, Sregion_end, 0, 0, 0,
-       doc: /* Return position of end of region, as an integer.  */)
+       doc: /* Return position of end of region, as an integer.
+
+The end of the region is point or mark, whichever is larger.  */)
      ()
 {
   return region_limit (0);

bug#6493:

[Drew Adams]

> How about this? (See attached patch.)

1. Thanks for working on this.

2. Please cc the bug submitter, and please put the bug subject line in the
Subject line.  It was only by searching old mails for "6493" that I sent that I
was able to find that this was a bug I submitted. Now I must search similarly
for the other bugs you replied to with no Subject line other than the bug #.
Multiply my experience by that of others.

3. What's important about these functions is the return value. The doc string
needs to be explicit about what is _returned_. There is no need to repeat info
that follows from definitions (e.g. that point and mark are the region limits).

Example:

"Returns point or mark, whichever is larger."

Thx.

bug#6493:

[Deniz Dogan]

2010/6/27 Drew Adams <drew.adams@oracle.com>:
>> How about this? (See attached patch.)
>
> 1. Thanks for working on this.
>
> 2. Please cc the bug submitter, and please put the bug subject line in the
> Subject line.  It was only by searching old mails for "6493" that I sent that I
> was able to find that this was a bug I submitted. Now I must search similarly
> for the other bugs you replied to with no Subject line other than the bug #.
> Multiply my experience by that of others.
>

Sorry about this, it just wasn't clear to me how people are supposed
to use the bug tracker. I'm not used to accessing bug trackers by
e-mail.

> 3. What's important about these functions is the return value. The doc string
> needs to be explicit about what is _returned_. There is no need to repeat info
> that follows from definitions (e.g. that point and mark are the region limits).
>
> Example:
>
> "Returns point or mark, whichever is larger."
>

How about this (for region-end):

"Returns point or mark, whichever is larger.

The returned value is the position of the end of the region as an integer."

...and likewise for region-beginning.

-- 
Deniz Dogan

bug#6493:

[Drew Adams]

> > "Returns point or mark, whichever is larger."
> 
> How about this (for region-end):
> 
> "Returns point or mark, whichever is larger.
> The returned value is the position of the end of the region 
> as an integer."

The end of the region is always a position, so no need to speak of the position
of the end of the region.

We don't really need to say that a number is returned, as opposed to a marker.
"Point" is a position, which is a number (natnump). "Mark" is more ambiguous -
it is sometimes referred to as a marker and sometimes as the position of that
marker.  So it's probably good to mention that a number is returned, even if not
really necessary. (We also need not specify what kind of number, since it is a
position, but that doesn't hurt either.) 

"Returns the larger of point or mark, as a nonnegative integer."

We can add that the function "raises an error if the mark is not set".

bug#6493:

[Deniz Dogan]

2010/6/27 Drew Adams <drew.adams@oracle.com>:
>> > "Returns point or mark, whichever is larger."
>>
>> How about this (for region-end):
>>
>> "Returns point or mark, whichever is larger.
>> The returned value is the position of the end of the region
>> as an integer."
>
> The end of the region is always a position, so no need to speak of the position
> of the end of the region.
>
> We don't really need to say that a number is returned, as opposed to a marker.
> "Point" is a position, which is a number (natnump). "Mark" is more ambiguous -
> it is sometimes referred to as a marker and sometimes as the position of that
> marker.  So it's probably good to mention that a number is returned, even if not
> really necessary. (We also need not specify what kind of number, since it is a
> position, but that doesn't hurt either.)
>
> "Returns the larger of point or mark, as a nonnegative integer."
>
> We can add that the function "raises an error if the mark is not set".
>
>

I'm all for your suggestion. So now we have:

"Returns the larger of point or mark, as a nonnegative integer.

Raises an error if the mark is not set."

Agreed?

-- 
Deniz Dogan

bug#6493:

[Drew Adams]

> I'm all for your suggestion. So now we have:
> "Returns the larger of point or mark, as a nonnegative integer.
> Raises an error if the mark is not set."
> Agreed?

Yes, thanks.

bug#6493:

[Deniz Dogan]

2010/6/27 Drew Adams <drew.adams@oracle.com>:
>> I'm all for your suggestion. So now we have:
>> "Returns the larger of point or mark, as a nonnegative integer.
>> Raises an error if the mark is not set."
>> Agreed?
>
> Yes, thanks.
>
>

I don't have commit access, Drew. Could you (or anyone else) make this
change now or are we waiting for more comments?

bug#6493:

[Drew Adams]

> >> I'm all for your suggestion. So now we have:
> >> "Returns the larger of point or mark, as a nonnegative integer.
> >> Raises an error if the mark is not set."
> >> Agreed?
> >
> > Yes, thanks.
> 
> I don't have commit access, Drew. Could you (or anyone else) make this
> change now or are we waiting for more comments?

Someone else will need to help here.  I have no idea whether I have commit
access, but I don't know how to commit or otherwise use the version-control
system.

I'm sure that sooner or later some kind soul will commit this. ;-)  Thanks for
working on it.

P.S. You might want to ask for commit access for yourself.  Dunno what the
process for that is.

bug#6493:

[Juanma Barranquero]

On Sun, Jul 11, 2010 at 06:20, Drew Adams <drew.adams@oracle.com> wrote:

> I'm sure that sooner or later some kind soul will commit this. ;-)  Thanks for
> working on it.

The discussion finished with no updated patch...

    Juanma

bug#6493:

[Deniz Dogan]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

2010/7/23 Juanma Barranquero <lekktu@gmail.com>:
> On Sun, Jul 11, 2010 at 06:20, Drew Adams <drew.adams@oracle.com> wrote:
>
>> I'm sure that sooner or later some kind soul will commit this. ;-)  Thanks for
>> working on it.
>
> The discussion finished with no updated patch...
>
>     Juanma
>

Attached is a new patch.

-- 
Deniz Dogan

[-- Attachment #2: editfns-patch-dd.patch --]
[-- Type: application/octet-stream, Size: 768 bytes --]

=== modified file 'src/editfns.c'
--- src/editfns.c	2010-06-16 20:08:41 +0000
+++ src/editfns.c	2010-06-27 12:22:00 +0000
@@ -349,14 +349,18 @@
 }
 
 DEFUN ("region-beginning", Fregion_beginning, Sregion_beginning, 0, 0, 0,
-       doc: /* Return position of beginning of region, as an integer.  */)
+       doc: /* Returns the smaller of point or mark, as a nonnegative integer.
+
+Raises an error if the mark is not set.  */)
      ()
 {
   return region_limit (1);
 }
 
 DEFUN ("region-end", Fregion_end, Sregion_end, 0, 0, 0,
-       doc: /* Return position of end of region, as an integer.  */)
+       doc: /* Returns the larger of point or mark, as a nonnegative integer.
+
+Raises an error if the mark is not set.  */)
      ()
 {
   return region_limit (0);

bug#6493:

[Juanma Barranquero]

On Fri, Jul 23, 2010 at 11:10, Deniz Dogan <deniz.a.m.dogan@gmail.com> wrote:

> Attached is a new patch.

Thanks.

Three comments:

 - You changed "Return" to "Returns", but it is standard Emacs
practice to make the first line in a docstring imperative. From
(elisp) D.6 "Tips for Documentation Strings":

  For consistency, phrase the verb in the first sentence of a
  function's documentation string as an imperative--for instance,
  use "Return the cons of A and B." in preference to "Returns the
  cons of A and B."

- You didn't write ChangeLog entries. They are trivial in this case
(so no need to resend), but usually a patch should include them.

- This thead has been just Drew and you, and I'm not sure the result
is better. Though I understand Drew's arguments, it is still a bit
weird to me to look at the docstring of region-(beginning|end) and not
to see *any* reference to the region (yes, I know it is implicit in
"point or mark"). So I'd like to hear more opinions.

    Juanma


=== modified file 'src/editfns.c'
--- src/editfns.c	2010-06-16 20:08:41 +0000
+++ src/editfns.c	2010-06-27 12:22:00 +0000
@@ -349,14 +349,18 @@
 }

 DEFUN ("region-beginning", Fregion_beginning, Sregion_beginning, 0, 0, 0,
-       doc: /* Return position of beginning of region, as an integer.  */)
+       doc: /* Returns the smaller of point or mark, as a nonnegative integer.
+
+Raises an error if the mark is not set.  */)
      ()
 {
   return region_limit (1);
 }

 DEFUN ("region-end", Fregion_end, Sregion_end, 0, 0, 0,
-       doc: /* Return position of end of region, as an integer.  */)
+       doc: /* Returns the larger of point or mark, as a nonnegative integer.
+
+Raises an error if the mark is not set.  */)
      ()
 {
   return region_limit (0);

bug#6493:

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Fri, Jul 23, 2010 at 11:10, Deniz Dogan <deniz.a.m.dogan@gmail.com> wrote:
>
>> Attached is a new patch.
>
> Thanks.
>
> Three comments:

Aside from these points (with which I agree), the addition of "as a
nonnegative integer" is unnecessary.  Point and mark are always
non-negative.  Also, it is important to keep the word "position",
because mark could otherwise refer to the mark marker.  I prefer:

  "Return the position of point or mark, whichever is smaller."

bug#6493:

[Drew Adams]

> I prefer:
>   "Return the position of point or mark, whichever is smaller."

Point _is_ a position.  It is the position of the cursor.  So that text speaks
about the position of the position of the cursor, which is incorrect.

But you are correct that we do speak of "setting the mark at a position".

bug#6493:

[Stephen Berman]

On Fri, 23 Jul 2010 11:45:13 -0400 Chong Yidong <cyd@stupidchicken.com> wrote:

> Juanma Barranquero <lekktu@gmail.com> writes:
>
>> On Fri, Jul 23, 2010 at 11:10, Deniz Dogan <deniz.a.m.dogan@gmail.com> wrote:
>>
>>> Attached is a new patch.
>>
>> Thanks.
>>
>> Three comments:
>
> Aside from these points (with which I agree), the addition of "as a
> nonnegative integer" is unnecessary.  Point and mark are always
> non-negative.  Also, it is important to keep the word "position",
> because mark could otherwise refer to the mark marker.  I prefer:
>
>   "Return the position of point or mark, whichever is smaller."

On Fri, 23 Jul 2010 09:16:48 -0700 "Drew Adams" <drew.adams@oracle.com> wrote:

> Point _is_ a position.  It is the position of the cursor.  So that text speaks
> about the position of the position of the cursor, which is incorrect.
>
> But you are correct that we do speak of "setting the mark at a position".

How about this:
"Return the smaller of the integer value of point or the mark."
(`value' is used in the doc strings of 'point' and `mark'.)

Steve Berman

bug#6493:

[Drew Adams]

> How about this:
> "Return the smaller of the integer value of point or the mark."
> (`value' is used in the doc strings of 'point' and `mark'.)

The scope of "the integer value of" is ambiguous.  Does it apply to "point or
the mark" or to just to "point"?  In the latter case, if the mark is smaller
than the integer value of point then the mark is returned.  (Yes, a marker can
be compared numerically.)

And in that case, it is incorrect.  The mark is a marker.  A marker is never
returned here - it is the integer value of the mark (or of point) that is
returned.

And in the former case, the grammar is incorrect - it should be "and" not "or".
It's `min' applied to a plurality (2) of things, not just to one of them.  So
"value" needs to be "values" and "or" needs to be "and".  Hence:

"Return the smaller of the integer values of point and mark."

But that could conceivably be misread to suggest that either can have multiple
values...

This is what I recommend (very close to what I mentioned in the beginning):

"Return the integer value of point or mark, whichever is smaller."

bug#6493:

[Juanma Barranquero]

On Fri, Jul 23, 2010 at 19:26, Drew Adams <drew.adams@oracle.com> wrote:

> "Return the integer value of point or mark, whichever is smaller."

FWIW, I like this one.

    Juanma

bug#6493:

[Juanma Barranquero]

The conclusion is that Chong thinks that "position" should be kept,
and Drew thinks that it is redundant.

I agree with Drew. The manual says clearly: "The active cursor shows
the location at which editing commands will
take effect, which is called "point"(1)."

So I'd like to commit his proposal ("Return the integer value of point
or mark, whichever is smaller.") and just get rid of this bug.

    Juanma

bug#6493:

[Johan Bockgård]

Juanma Barranquero <lekktu@gmail.com> writes:

> So I'd like to commit his proposal ("Return the integer value of point
> or mark, whichever is smaller.") and just get rid of this bug.

Something that should be mentioned in the doc strings is the following:

    The next two functions signal an error if the mark does not point
    anywhere. If Transient Mark mode is enabled and
    `mark-even-if-inactive' is `nil', they also signal an error if the
    mark is inactive.

bug#6493:

[Juanma Barranquero]

On Thu, Jul 29, 2010 at 17:16, Johan Bockgård <bojohan@gnu.org> wrote:

> Something that should be mentioned in the doc strings is the following:
>
>    The next two functions signal an error if the mark does not point
>    anywhere. If Transient Mark mode is enabled and
>    `mark-even-if-inactive' is `nil', they also signal an error if the
>    mark is inactive.

Can we simplify that to just say that they signal an error if the
region is not active? (The region-* commands are somewhat specific, so
it's reasonable to suppose that the programmer knows what he's doing
or can lookup the docs.)

    Juanma

bug#6493:

[Drew Adams]

> > Something that should be mentioned in the doc strings is 
> > the following:
> >
> >    The next two functions signal an error if the mark does not point
> >    anywhere. If Transient Mark mode is enabled and
> >    `mark-even-if-inactive' is `nil', they also signal an 
> >    error if the mark is inactive.

Why should that be mentioned?
What important problem or use case would we be pointing out?

> Can we simplify that to just say that they signal an error if the
> region is not active? (The region-* commands are somewhat specific, so
> it's reasonable to suppose that the programmer knows what he's doing
> or can lookup the docs.)

I have no real objection to a clear statement of the error handling.
In general it is a good idea to mention important error cases.

But I do not think it is necessary to mention error handling at all for these
doc strings.  In this case, saying more is saying less, and likely to just add
confusion.

It's pretty obvious that a numerical comparison needs to be done to determine
the smaller/larger of point and mark, and it's similarly obvious that if the
numerical value of one of them (mark) cannot be determined then an error would
be raised.

That does not follow _logically_ - we could return the other value (point is
numeric) or some special value to indicate this condition, but raising an error
seems like common sense and what users would expect.  I see no reason to add
info about the error handling.

bug#6493:

[Juanma Barranquero]

2010/8/1 Drew Adams <drew.adams@oracle.com>:

> That does not follow _logically_ - we could return the other value (point is
> numeric) or some special value to indicate this condition, but raising an error
> seems like common sense and what users would expect.

That's highly subjective IMHO.

Do you oppose to the following (minimal) version?

    Juanma


=== modified file 'src/editfns.c'
--- src/editfns.c	2010-07-29 15:50:04 +0000
+++ src/editfns.c	2010-08-02 03:27:32 +0000
@@ -337,5 +337,6 @@

 DEFUN ("region-beginning", Fregion_beginning, Sregion_beginning, 0, 0, 0,
-       doc: /* Return position of beginning of region, as an integer.  */)
+       doc: /* Return the integer value of point or mark, whichever is smaller.
+Signal an error if the region is not active.  */)
   (void)
 {
@@ -344,5 +345,6 @@

 DEFUN ("region-end", Fregion_end, Sregion_end, 0, 0, 0,
-       doc: /* Return position of end of region, as an integer.  */)
+       doc: /* Return the integer value of point or mark, whichever is larger.
+Signal an error if the region is not active.  */)
   (void)
 {

bug#6493:

[Drew Adams]

> > an error seems like common sense and what users would expect.
> 
> That's highly subjective IMHO.
> Do you oppose to the following (minimal) version?

Nope.  (I already said so.)

bug#6493:

[Drew Adams]

> > > an error seems like common sense and what users would expect.
> > 
> > That's highly subjective IMHO.
> > Do you oppose to the following (minimal) version?
> 
> Nope.  (I already said so.)

Oops, yes, I do oppose it, because it is incorrect.

What I said earlier was:
 "I have no real objection to a clear statement of the error handling."
                                ^^^^^^^^^^^^^^^

But it is incorrect that these raise an error if the region is inactive - they
do not.

emacs -Q

Visit some file.
C-SPC, then move point somewhere.
C-g to deactivate the region.
M-: (region-beginning) -> returns the value of `(mark)'.

This is with the default value of `mark-even-if-inactive' and the default of
transient-mark-mode turned on.

If you want to describe the error treatment then you need to do so completely
and correctly.  As before, I recommend against describing error handling in this
case, but I do not oppose it if done right.

bug#6493:

[Juanma Barranquero]

On Mon, Aug 2, 2010 at 07:34, Drew Adams <drew.adams@oracle.com> wrote:

> Oops, yes, I do oppose it, because it is incorrect.

Committed without the error note.

> If you want to describe the error treatment then you need to do so completely
> and correctly.  As before, I recommend against describing error handling in this
> case, but I do not oppose it if done right.

I'l leave that to interested parties.

    Juanma

bug#6493:

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

> The conclusion is that Chong thinks that "position" should be kept,
> and Drew thinks that it is redundant.
>
> I agree with Drew. The manual says clearly: "The active cursor shows
> the location at which editing commands will
> take effect, which is called "point"(1)."
>
> So I'd like to commit his proposal ("Return the integer value of point
> or mark, whichever is smaller.") and just get rid of this bug.

Go ahead.