bug#35062: Patches: trivial cleanups

bug#35062: Patches: trivial cleanups

[Konstantin Kharlamov]

Per discussion on emacs-devel, this topic is created so that following 
patches wouldn't create separate reports.

bug#35062: [PATCH 0/4] Trivial code cleanups

[Konstantin Kharlamov]

These are mostly fixes of some of LGTM warnings
https://lgtm.com/projects/g/emacs-mirror/emacs/alerts/?mode=tree

Except the second patch, where I initially wanted to fix one warning,
and as part of it I had to constify a variable to see that it is indeed
immutable. And then I figured I could search through the code and find
more similar places, where variables weren't marked as const. I like
this cleanup because it is simple and trivially testable (i.e. if it
compiles, then it's fine). FTR: there's still lots of opportunities for
constification, I just stopped at some point.

Konstantin Kharlamov (4):
  Remove redundant comparison
  constify a bit of xterm.c
  min_cols/rows is always 0, don't check for it
  don't compare unsigned to less-than-zero

 src/gtkutil.c |  5 +----
 src/pdumper.c |  2 --
 src/xterm.c   | 34 +++++++++++++++++-----------------
 3 files changed, 18 insertions(+), 23 deletions(-)

-- 
2.21.0

bug#35062: [PATCH 1/4] Remove redundant comparison

[Konstantin Kharlamov]

* src/xterm.c: due to preceding checks it is bound to be 0 ≤ alpha ≤ 1.

Fixes LGTM warning: comparison is always true because 0 <= alpha.
---
 src/xterm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xterm.c b/src/xterm.c
index f90d6713b02..e8f1e576a38 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -931,7 +931,7 @@ x_set_frame_alpha (struct frame *f)
     return;
   else if (alpha > 1.0)
     alpha = 1.0;
-  else if (0.0 <= alpha && alpha < alpha_min && alpha_min <= 1.0)
+  else if (alpha < alpha_min && alpha_min <= 1.0)
     alpha = alpha_min;
 
   opac = alpha * OPAQUE;
-- 
2.21.0

bug#35062: [PATCH 2/4] constify a bit of xterm.c

[Konstantin Kharlamov]

* src/xterm.c: make code easier to follow by making explicit that some
variables are immutable
---
 src/xterm.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/xterm.c b/src/xterm.c
index e8f1e576a38..8354ce00700 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -495,9 +495,9 @@ x_cr_draw_image (struct frame *f, GC gc, cairo_surface_t *image,
       cairo_fill_preserve (cr);
     }
 
-  int orig_image_width = cairo_image_surface_get_width (image);
+  const int orig_image_width = cairo_image_surface_get_width (image);
   if (image_width == 0) image_width = orig_image_width;
-  int orig_image_height = cairo_image_surface_get_height (image);
+  const int orig_image_height = cairo_image_surface_get_height (image);
   if (image_height == 0) image_height = orig_image_height;
 
   cairo_pattern_t *pattern = cairo_pattern_create_for_surface (image);
@@ -1006,7 +1006,7 @@ x_update_begin (struct frame *f)
       if (FRAME_GTK_WIDGET (f))
         {
           GdkWindow *w = gtk_widget_get_window (FRAME_GTK_WIDGET (f));
-	  int scale = xg_get_scale (f);
+	  const int scale = xg_get_scale (f);
 	  width = scale * gdk_window_get_width (w);
 	  height = scale * gdk_window_get_height (w);
         }
@@ -1300,15 +1300,15 @@ x_clear_under_internal_border (struct frame *f)
 {
   if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0)
     {
-      int border = FRAME_INTERNAL_BORDER_WIDTH (f);
-      int width = FRAME_PIXEL_WIDTH (f);
-      int height = FRAME_PIXEL_HEIGHT (f);
+      const int border = FRAME_INTERNAL_BORDER_WIDTH (f);
+      const int width = FRAME_PIXEL_WIDTH (f);
+      const int height = FRAME_PIXEL_HEIGHT (f);
 #ifdef USE_GTK
-      int margin = 0;
+      const int margin = 0;
 #else
-      int margin = FRAME_TOP_MARGIN_HEIGHT (f);
+      const int margin = FRAME_TOP_MARGIN_HEIGHT (f);
 #endif
-      int face_id =
+      const int face_id =
 	!NILP (Vface_remapping_alist)
 	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
 	: INTERNAL_BORDER_FACE_ID;
@@ -1455,7 +1455,7 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
       Drawable drawable = FRAME_X_DRAWABLE (f);
       char *bits;
       Pixmap pixmap, clipmask = (Pixmap) 0;
-      int depth = DefaultDepthOfScreen (FRAME_X_SCREEN (f));
+      const int depth = DefaultDepthOfScreen (FRAME_X_SCREEN (f));
       XGCValues gcv;
 
       if (p->wd > 8)
@@ -1704,7 +1704,7 @@ static void
 x_set_glyph_string_clipping (struct glyph_string *s)
 {
   XRectangle *r = s->clip;
-  int n = get_glyph_string_clip_rects (s, r, 2);
+  const int n = get_glyph_string_clip_rects (s, r, 2);
 
   if (n > 0)
     x_set_clip_rectangles (s->f, s->gc, r, n);
@@ -1797,7 +1797,7 @@ x_draw_glyph_string_background (struct glyph_string *s, bool force_p)
      shouldn't be drawn in the first place.  */
   if (!s->background_filled_p)
     {
-      int box_line_width = max (s->face->box_line_width, 0);
+      const int box_line_width = max (s->face->box_line_width, 0);
 
       if (s->stippled_p)
 	{
@@ -1908,15 +1908,15 @@ x_draw_composite_glyph_string_foreground (struct glyph_string *s)
     }
   else if (! s->first_glyph->u.cmp.automatic)
     {
-      int y = s->ybase;
+      const int y = s->ybase;
 
       for (i = 0, j = s->cmp_from; i < s->nchars; i++, j++)
 	/* TAB in a composition means display glyphs with padding
 	   space on the left or right.  */
 	if (COMPOSITION_GLYPH (s->cmp, j) != '\t')
 	  {
-	    int xx = x + s->cmp->offsets[j * 2];
-	    int yy = y - s->cmp->offsets[j * 2 + 1];
+	    const int xx = x + s->cmp->offsets[j * 2];
+	    const int yy = y - s->cmp->offsets[j * 2 + 1];
 
 	    font->driver->draw (s, j, j + 1, xx, yy, false);
 	    if (s->face->overstrike)
@@ -5516,7 +5516,7 @@ x_send_scroll_bar_event (Lisp_Object window, enum scroll_bar_part part,
   struct frame *f = XFRAME (w->frame);
   intptr_t iw = (intptr_t) w;
   verify (INTPTR_WIDTH <= 64);
-  int sign_shift = INTPTR_WIDTH - 32;
+  const int sign_shift = INTPTR_WIDTH - 32;
 
   block_input ();
 
-- 
2.21.0

bug#35062: [PATCH 3/4] min_cols/rows is always 0, don't check for it

[Konstantin Kharlamov]

* src/gtkutil.c: remove unnecessary comparison

Fixes LGTM warnings:
    * Comparison is always false because min_cols <= 0.
    * Comparison is always false because min_rows <= 0.
---
 src/gtkutil.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index 4bd73b1a6d1..64bc248d650 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -1401,7 +1401,7 @@ x_wm_set_size_hint (struct frame *f, long int flags, bool user_position)
   GdkGeometry size_hints;
   gint hint_flags = 0;
   int base_width, base_height;
-  int min_rows = 0, min_cols = 0;
+  const int min_rows = 0, min_cols = 0;
   int win_gravity = f->win_gravity;
   Lisp_Object fs_state, frame;
   int scale = xg_get_scale (f);
@@ -1450,9 +1450,6 @@ x_wm_set_size_hint (struct frame *f, long int flags, bool user_position)
   base_height = FRAME_TEXT_LINES_TO_PIXEL_HEIGHT (f, 1)
     + FRAME_MENUBAR_HEIGHT (f) + FRAME_TOOLBAR_HEIGHT (f);
 
-  if (min_cols > 0) --min_cols; /* We used one col in base_width = ... 1); */
-  if (min_rows > 0) --min_rows; /* We used one row in base_height = ... 1); */
-
   size_hints.base_width = base_width;
   size_hints.base_height = base_height;
   size_hints.min_width  = base_width + min_cols * FRAME_COLUMN_WIDTH (f);
-- 
2.21.0

bug#35062: [PATCH 3/4] min_cols/rows is always 0, don't check for it

[Noam Postavsky]

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> * src/gtkutil.c: remove unnecessary comparison

> @@ -1401,7 +1401,7 @@ x_wm_set_size_hint (struct frame *f, long int flags, bool user_position)

> -  int min_rows = 0, min_cols = 0;
> +  const int min_rows = 0, min_cols = 0;

> -  if (min_cols > 0) --min_cols; /* We used one col in base_width = ... 1); */
> -  if (min_rows > 0) --min_rows; /* We used one row in base_height = ... 1); */
> -
>    size_hints.base_width = base_width;
>    size_hints.base_height = base_height;
>    size_hints.min_width  = base_width + min_cols * FRAME_COLUMN_WIDTH (f);

If min_cols is always 0, then what use is the
    "+ min_cols * FRAME_COLUMN_WIDTH (f)"?

(and a similar question arises for min_cols and min_rows in
update_wm_hints of widget.c)

bug#35062: [PATCH 3/4] min_cols/rows is always 0, don't check for it

[Konstantin Kharlamov]

В Пн, апр 1, 2019 at 18:37, Noam Postavsky <npostavs@gmail.com> 
написал:
> Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:
> 
>>  * src/gtkutil.c: remove unnecessary comparison
> 
>>  @@ -1401,7 +1401,7 @@ x_wm_set_size_hint (struct frame *f, long int 
>> flags, bool user_position)
> 
>>  -  int min_rows = 0, min_cols = 0;
>>  +  const int min_rows = 0, min_cols = 0;
> 
>>  -  if (min_cols > 0) --min_cols; /* We used one col in base_width = 
>> ... 1); */
>>  -  if (min_rows > 0) --min_rows; /* We used one row in base_height 
>> = ... 1); */
>>  -
>>     size_hints.base_width = base_width;
>>     size_hints.base_height = base_height;
>>     size_hints.min_width  = base_width + min_cols * 
>> FRAME_COLUMN_WIDTH (f);
> 
> If min_cols is always 0, then what use is the
>     "+ min_cols * FRAME_COLUMN_WIDTH (f)"?
> 
> (and a similar question arises for min_cols and min_rows in
> update_wm_hints of widget.c)

Good point. I did some digging in git-log/git-blame: once upon a time 
there was a check_frame_size (f, &min_cols, &min_rows, 0) call. Then in 
commit 3477e27021dbe9366c3c1aaba80feb72f1138b29 for some reason this 
call was removed. Reason is unclear because the description only says 
multiple times "remove check_frame_size", and the commit itself is so 
*GIGANTIC* that when I held PgDown, it took me 9 seconds just to scroll 
it to the bottom on a 1366×768 display. But the commit is dated by 
2014, so given it's still there, let's hope it's correct.

Then I guess, it makes sense to remove the multiplication by zero, 
because it's a noop anyway. I gonna make changes in gtkutil.c in some 
minutes, but what do I do with widget.c? Do I just send the patch here? 
I'm asking because Emacs has unusual workflow regarding patches 
sending. E.g. in other mailing-list based projects I'd just resend the 
series. In gitlab/github it's much simpler: I just push the new commit. 
But in Emacs one has to create a debbugs report, then send patches 
there; and I already have this "report" so I'm confused.

bug#35062: [PATCH 3/4] min_cols/rows is always 0, don't check for it

[Eli Zaretskii]

> Date: Tue, 02 Apr 2019 03:09:14 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: 35062@debbugs.gnu.org
> 
> what do I do with widget.c? Do I just send the patch here? 
> I'm asking because Emacs has unusual workflow regarding patches 
> sending. E.g. in other mailing-list based projects I'd just resend the 
> series. In gitlab/github it's much simpler: I just push the new commit. 
> But in Emacs one has to create a debbugs report, then send patches 
> there; and I already have this "report" so I'm confused.

The rule is very simple: if the other change is for the same or a
similar issue, just send another patch to the same issue ticket;
otherwise file a separate issue.  (But no one will be necessarily mad
at you if you send to the same ticket anyway, it just runs a higher
risk of falling through cracks.  IOW, don't worry too much about this
stuff, just use common sense; we are not as rigid in our procedures as
some other projects.)

bug#35062: [PATCH 3/4] min_cols/rows is always 0, don't check for it

[Konstantin Kharlamov]

Thanks, done!

In other news: today I got a confirmation from FSF that assignment 
process is complete. I guess that's it, I can officially contribute to 
Emacs now…?

В Вт, апр 2, 2019 at 17:46, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  Date: Tue, 02 Apr 2019 03:09:14 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: 35062@debbugs.gnu.org
>> 
>>  what do I do with widget.c? Do I just send the patch here?
>>  I'm asking because Emacs has unusual workflow regarding patches
>>  sending. E.g. in other mailing-list based projects I'd just resend 
>> the
>>  series. In gitlab/github it's much simpler: I just push the new 
>> commit.
>>  But in Emacs one has to create a debbugs report, then send patches
>>  there; and I already have this "report" so I'm confused.
> 
> The rule is very simple: if the other change is for the same or a
> similar issue, just send another patch to the same issue ticket;
> otherwise file a separate issue.  (But no one will be necessarily mad
> at you if you send to the same ticket anyway, it just runs a higher
> risk of falling through cracks.  IOW, don't worry too much about this
> stuff, just use common sense; we are not as rigid in our procedures as
> some other projects.)

bug#35062: [PATCH 3/4] min_cols/rows is always 0, don't check for it

[Eli Zaretskii]

> Date: Tue, 02 Apr 2019 23:54:42 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: npostavs@gmail.com, 35062@debbugs.gnu.org
> 
> In other news: today I got a confirmation from FSF that assignment 
> process is complete. I guess that's it, I can officially contribute to 
> Emacs now…?

Yes, welcome on board.

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Konstantin Kharlamov]

* src/gtkutil.c:
    remove "always false" comparison
    remove multiplication by zero, it's a noop anyway
    remove min_cols and min_rows as it's now unused

Fixes LGTM warnings:
    * Comparison is always false because min_cols <= 0.
    * Comparison is always false because min_rows <= 0.
---
v2: remove the min_rows/min_cols whatsoever

 src/gtkutil.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index 4bd73b1a6d1..b130692c87a 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -1401,7 +1401,6 @@ x_wm_set_size_hint (struct frame *f, long int flags, bool user_position)
   GdkGeometry size_hints;
   gint hint_flags = 0;
   int base_width, base_height;
-  int min_rows = 0, min_cols = 0;
   int win_gravity = f->win_gravity;
   Lisp_Object fs_state, frame;
   int scale = xg_get_scale (f);
@@ -1450,13 +1449,10 @@ x_wm_set_size_hint (struct frame *f, long int flags, bool user_position)
   base_height = FRAME_TEXT_LINES_TO_PIXEL_HEIGHT (f, 1)
     + FRAME_MENUBAR_HEIGHT (f) + FRAME_TOOLBAR_HEIGHT (f);
 
-  if (min_cols > 0) --min_cols; /* We used one col in base_width = ... 1); */
-  if (min_rows > 0) --min_rows; /* We used one row in base_height = ... 1); */
-
   size_hints.base_width = base_width;
   size_hints.base_height = base_height;
-  size_hints.min_width  = base_width + min_cols * FRAME_COLUMN_WIDTH (f);
-  size_hints.min_height = base_height + min_rows * FRAME_LINE_HEIGHT (f);
+  size_hints.min_width  = base_width;
+  size_hints.min_height = base_height;
 
   /* These currently have a one to one mapping with the X values, but I
      don't think we should rely on that.  */
-- 
2.21.0

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Tue,  2 Apr 2019 03:23:27 +0300
> 
> * src/gtkutil.c:
>     remove "always false" comparison
>     remove multiplication by zero, it's a noop anyway
>     remove min_cols and min_rows as it's now unused
> 
> Fixes LGTM warnings:
>     * Comparison is always false because min_cols <= 0.
>     * Comparison is always false because min_rows <= 0.
> ---
> v2: remove the min_rows/min_cols whatsoever

Thanks, pushed.  Please in the future write the commit log messages in
the ChangeLog-like style, see CONTRIBUTE for the details.  I did that
this time, you can look at what I pushed to see an example of that
style in action.

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Konstantin Kharlamov]

On Пт, Apr 5, 2019 at 10:05, Eli Zaretskii <eliz@gnu.org> wrote:
>>  From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>>  Date: Tue,  2 Apr 2019 03:23:27 +0300
>> 
>>  * src/gtkutil.c:
>>      remove "always false" comparison
>>      remove multiplication by zero, it's a noop anyway
>>      remove min_cols and min_rows as it's now unused
>> 
>>  Fixes LGTM warnings:
>>      * Comparison is always false because min_cols <= 0.
>>      * Comparison is always false because min_rows <= 0.
>>  ---
>>  v2: remove the min_rows/min_cols whatsoever
> 
> Thanks, pushed.  Please in the future write the commit log messages in
> the ChangeLog-like style, see CONTRIBUTE for the details.  I did that
> this time, you can look at what I pushed to see an example of that
> style in action.

Thanks, so, I'm looking right now to modify the rest of series and 
resend it here, and… I think I miss something: the only difference of 
my commit from CONTRIBUTE file is that I forgot to list the functions 
changed.

I think I misunderstand something, because you completely rewrote the 
commit messages compared to mine. Titles now are more vague, and for 
some reason you removed a note that one of patches fixes warnings in 
the code. And then you also added "Bug#35062", even though there's no 
bug (the report existence is incidental, it's because bugtracker is 
used to track patches in Emacs. But writing "Bug" will only confuse 
readers — there's no bug).

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Eli Zaretskii]

> Date: Sat, 06 Apr 2019 01:18:38 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: 35062@debbugs.gnu.org
> 
> >>  * src/gtkutil.c:
> >>      remove "always false" comparison
> >>      remove multiplication by zero, it's a noop anyway
> >>      remove min_cols and min_rows as it's now unused
> >> 
> >>  Fixes LGTM warnings:
> >>      * Comparison is always false because min_cols <= 0.
> >>      * Comparison is always false because min_rows <= 0.
> >>  ---
> >>  v2: remove the min_rows/min_cols whatsoever
> > 
> > Thanks, pushed.  Please in the future write the commit log messages in
> > the ChangeLog-like style, see CONTRIBUTE for the details.  I did that
> > this time, you can look at what I pushed to see an example of that
> > style in action.
> 
> Thanks, so, I'm looking right now to modify the rest of series and 
> resend it here, and… I think I miss something: the only difference of 
> my commit from CONTRIBUTE file is that I forgot to list the functions 
> changed.

That's right.  And also the lack of the bug number, see below.

> I think I misunderstand something, because you completely rewrote the 
> commit messages compared to mine. Titles now are more vague, and for 
> some reason you removed a note that one of patches fixes warnings in 
> the code.

That's just my personal preferences of style, as there's always a
judgment call regarding the description of the changes.  We don't have
to agree about that, and if your log message was in the right format,
I probably wouldn't have bothered changing their text.

I can explain my preference: your text described in a very detailed
form something that was acutely visible from the diffs themselves,
whereas my text only describes the motivation, leaving the rest to the
diffs which speak for themselves.

The part about LGTM warnings didn't seem important enough to me: LGTM
is not a compiler, and without a reference to what it is, the text is
a small riddle in itself.  More importantly, I don't think we need any
justification for removing variables initialized to zero and never
changed.

Again, these are my personal preferences, not something we require
from each contributor.  So if you disagree, you can keep your style,
and only adjust the form.

> And then you also added "Bug#35062", even though there's no 
> bug (the report existence is incidental, it's because bugtracker is 
> used to track patches in Emacs.

References to bug numbers should always be in the log message, because
that allows an easy way of reading relevant discussions recorded by
the bug tracker.  This _is_ in CONTRIBUTE, btw.

> But writing "Bug" will only confuse readers — there's no bug).

"Bug" here doesn't mean a literal bug, it means an issue recorded by
the bug tracker.

Thanks.

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Konstantin Kharlamov]

В Сб, апр 6, 2019 at 10:09, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  Date: Sat, 06 Apr 2019 01:18:38 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: 35062@debbugs.gnu.org
>> 
>>  >>  * src/gtkutil.c:
>>  >>      remove "always false" comparison
>>  >>      remove multiplication by zero, it's a noop anyway
>>  >>      remove min_cols and min_rows as it's now unused
>>  >>
>>  >>  Fixes LGTM warnings:
>>  >>      * Comparison is always false because min_cols <= 0.
>>  >>      * Comparison is always false because min_rows <= 0.
>>  >>  ---
>>  >>  v2: remove the min_rows/min_cols whatsoever
>>  >
>>  > Thanks, pushed.  Please in the future write the commit log 
>> messages in
>>  > the ChangeLog-like style, see CONTRIBUTE for the details.  I did 
>> that
>>  > this time, you can look at what I pushed to see an example of that
>>  > style in action.
>> 
>>  Thanks, so, I'm looking right now to modify the rest of series and
>>  resend it here, and… I think I miss something: the only 
>> difference of
>>  my commit from CONTRIBUTE file is that I forgot to list the 
>> functions
>>  changed.
> 
> That's right.  And also the lack of the bug number, see below.
> 
>>  I think I misunderstand something, because you completely rewrote 
>> the
>>  commit messages compared to mine. Titles now are more vague, and for
>>  some reason you removed a note that one of patches fixes warnings in
>>  the code.
> 
> That's just my personal preferences of style, as there's always a
> judgment call regarding the description of the changes.  We don't have
> to agree about that, and if your log message was in the right format,
> I probably wouldn't have bothered changing their text.
> 
> I can explain my preference: your text described in a very detailed
> form something that was acutely visible from the diffs themselves,
> whereas my text only describes the motivation, leaving the rest to the
> diffs which speak for themselves.

Thanks!

> The part about LGTM warnings didn't seem important enough to me: LGTM
> is not a compiler, and without a reference to what it is, the text is
> a small riddle in itself.  More importantly, I don't think we need any
> justification for removing variables initialized to zero and never
> changed.

Well, one can find what LGTM through a search engine, or for that 
matter by using "BUG#XXX" to find this discussion. But whatever.

> Again, these are my personal preferences, not something we require
> from each contributor.  So if you disagree, you can keep your style,
> and only adjust the form.
> 
>>  And then you also added "Bug#35062", even though there's no
>>  bug (the report existence is incidental, it's because bugtracker is
>>  used to track patches in Emacs.
> 
> References to bug numbers should always be in the log message, because
> that allows an easy way of reading relevant discussions recorded by
> the bug tracker.  This _is_ in CONTRIBUTE, btw.

Oh, thanks, I missed that.

>>  But writing "Bug" will only confuse readers — there's no bug).
> 
> "Bug" here doesn't mean a literal bug, it means an issue recorded by
> the bug tracker.

I see, still it doesn't mean arbitrary readers won't be confused. 
git-logs are being read by lots of different people, most of them are 
not even contributors, could be e.g. users wondering whether it's worth 
to re-build their Emacs-from-git.

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Eli Zaretskii]

> Date: Sun, 07 Apr 2019 05:03:33 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: 35062@debbugs.gnu.org
> 
> >>  But writing "Bug" will only confuse readers — there's no bug).
> > 
> > "Bug" here doesn't mean a literal bug, it means an issue recorded by
> > the bug tracker.
> 
> I see, still it doesn't mean arbitrary readers won't be confused. 
> git-logs are being read by lots of different people, most of them are 
> not even contributors, could be e.g. users wondering whether it's worth 
> to re-build their Emacs-from-git.

These references are for the Emacs developers, the rest of the world
will have to adapt.  It's not a big deal anyway.

bug#35062: [PATCH v2] min_cols/rows is always 0, remove noop actions

[Michael Albinus]

Konstantin Kharlamov <hi-angel@yandex.ru> writes:

Hi Konstantin,

> And then you also added "Bug#35062", even though there's no bug (the
> report existence is incidental, it's because bugtracker is used to
> track patches in Emacs. But writing "Bug" will only confuse readers —
> there's no bug).

"Bug#nnnnn" is a pattern to identify references to debbugs.gnu.org in
files or git logs. Used by functions like `bug-reference-mode' or
`debbugs-browse-mode'.

Best regards, Michael.

bug#35062: [PATCH 4/4] don't compare unsigned to less-than-zero

[Konstantin Kharlamov]

* pdumper.c: don't compare unsigned to less-than-zero

Fixes 2 LGTM warnings: "Pointless comparison of unsigned value to zero."
---
 src/pdumper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index a9b3732a2d4..5407154fb2d 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -4434,7 +4434,6 @@ dump_anonymous_allocate (void *base,
 static void
 dump_anonymous_release (void *addr, size_t size)
 {
-  eassert (size >= 0);
 #if VM_SUPPORTED == VM_MS_WINDOWS
   (void) size;
   if (!VirtualFree (addr, 0, MEM_RELEASE))
@@ -4584,7 +4583,6 @@ dump_map_file (void *base, int fd, off_t offset, size_t size,
 static void
 dump_unmap_file (void *addr, size_t size)
 {
-  eassert (size >= 0);
 #if !VM_SUPPORTED
   (void) addr;
   (void) size;
-- 
2.21.0

bug#35062: [PATCH 0/4] Trivial code cleanups

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Mon,  1 Apr 2019 01:37:38 +0300
> 
> These are mostly fixes of some of LGTM warnings
> https://lgtm.com/projects/g/emacs-mirror/emacs/alerts/?mode=tree
> 
> Except the second patch, where I initially wanted to fix one warning,
> and as part of it I had to constify a variable to see that it is indeed
> immutable. And then I figured I could search through the code and find
> more similar places, where variables weren't marked as const. I like
> this cleanup because it is simple and trivially testable (i.e. if it
> compiles, then it's fine). FTR: there's still lots of opportunities for
> constification, I just stopped at some point.

Thanks.

I think the general policy is not to fix those except when making
other changes in the same function, but I will let others comment.

bug#35062: [PATCH 0/4] Trivial code cleanups

[Robert Pluim]

>>>>> On Mon, 01 Apr 2019 07:37:23 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Konstantin Kharlamov <Hi-Angel@yandex.ru> Date: Mon, 1
    >> Apr 2019 01:37:38 +0300
    >> 
    >> These are mostly fixes of some of LGTM warnings
    >> https://lgtm.com/projects/g/emacs-mirror/emacs/alerts/?mode=tree
    >> 
    >> Except the second patch, where I initially wanted to fix one
    >> warning, and as part of it I had to constify a variable to see
    >> that it is indeed immutable. And then I figured I could search
    >> through the code and find more similar places, where variables
    >> weren't marked as const. I like this cleanup because it is
    >> simple and trivially testable (i.e. if it compiles, then it's
    >> fine). FTR: there's still lots of opportunities for
    >> constification, I just stopped at some point.

    Eli> Thanks.

    Eli> I think the general policy is not to fix those except when
    Eli> making other changes in the same function, but I will let
    Eli> others comment.

Iʼd prefer it if the effort went to determining if eg the alert for
'type = 2' below was correct or not, proving the constness of
variables is what we have a compiler for.

xterm.c:5346

	if (XSCROLL_BAR (bar)->x_window == window_id
            && FRAME_X_DISPLAY (XFRAME (frame)) == display
	    && (type = 2
		|| (type == 1 && XSCROLL_BAR (bar)->horizontal)
		|| (type == 0 && !XSCROLL_BAR (bar)->horizontal)))
	  return XSCROLL_BAR (bar);

Robert

bug#35062: [PATCH 0/4] Trivial code cleanups

[Konstantin Kharlamov]

On Пн, Apr 1, 2019 at 15:27, Robert Pluim <rpluim@gmail.com> wrote:
>>>>>>  On Mon, 01 Apr 2019 07:37:23 +0300, Eli Zaretskii 
>>>>>> <eliz@gnu.org> said:
> 
>     >> From: Konstantin Kharlamov <Hi-Angel@yandex.ru> Date: Mon, 1
>     >> Apr 2019 01:37:38 +0300
>     >>
>     >> These are mostly fixes of some of LGTM warnings
>     >> 
> https://lgtm.com/projects/g/emacs-mirror/emacs/alerts/?mode=tree
>     >>
>     >> Except the second patch, where I initially wanted to fix one
>     >> warning, and as part of it I had to constify a variable to see
>     >> that it is indeed immutable. And then I figured I could search
>     >> through the code and find more similar places, where variables
>     >> weren't marked as const. I like this cleanup because it is
>     >> simple and trivially testable (i.e. if it compiles, then it's
>     >> fine). FTR: there's still lots of opportunities for
>     >> constification, I just stopped at some point.
> 
>     Eli> Thanks.
> 
>     Eli> I think the general policy is not to fix those except when
>     Eli> making other changes in the same function, but I will let
>     Eli> others comment.
> 
> Iʼd prefer it if the effort went to determining if eg the alert for
> 'type = 2' below was correct or not, proving the constness of
> variables is what we have a compiler for.
> 
> xterm.c:5346
> 
> 	if (XSCROLL_BAR (bar)->x_window == window_id
>             && FRAME_X_DISPLAY (XFRAME (frame)) == display
> 	    && (type = 2
> 		|| (type == 1 && XSCROLL_BAR (bar)->horizontal)
> 		|| (type == 0 && !XSCROLL_BAR (bar)->horizontal)))
> 	  return XSCROLL_BAR (bar);
> 
> Robert

Well, not everything at once! :) I afraid that if I fix lots of 
warnings in one patch-set, it may get stuck in review because of the 
amount of changes; besides it's easier for my sanity to send small 
patchsets because mailing-list based projects in general tend not to 
accept patches too quickly.

Also note: the constness here is not for compiler but for developers.

bug#35062: [PATCH 0/4] Trivial code cleanups

[Konstantin Kharlamov]

Btw, I'm okay if somebody else too gonna take down more warnings from 
the lgtm site :)

On Пн, Apr 1, 2019 at 16:35, Konstantin Kharlamov 
<hi-angel@yandex.ru> wrote:
> 
> 
> On Пн, Apr 1, 2019 at 15:27, Robert Pluim <rpluim@gmail.com> wrote:
>>>>>>>  On Mon, 01 Apr 2019 07:37:23 +0300, Eli Zaretskii 
>>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f<eliz@gnu.org> said:
>> 
>>     >> From: Konstantin Kharlamov <Hi-Angel@yandex.ru> Date: Mon, 1
>>     >> Apr 2019 01:37:38 +0300
>>     >>
>>     >> These are mostly fixes of some of LGTM warnings
>>     >> 
>> \x7fLINKIFYbJDaEcHBaBEaGEAEIJcfeafcJDJFFIJeffEEeEDE
>>     >>
>>     >> Except the second patch, where I initially wanted to fix one
>>     >> warning, and as part of it I had to constify a variable to see
>>     >> that it is indeed immutable. And then I figured I could search
>>     >> through the code and find more similar places, where variables
>>     >> weren't marked as const. I like this cleanup because it is
>>     >> simple and trivially testable (i.e. if it compiles, then it's
>>     >> fine). FTR: there's still lots of opportunities for
>>     >> constification, I just stopped at some point.
>> 
>>     Eli> Thanks.
>> 
>>     Eli> I think the general policy is not to fix those except when
>>     Eli> making other changes in the same function, but I will let
>>     Eli> others comment.
>> 
>> Iʼd prefer it if the effort went to determining if eg the alert for
>> 'type = 2' below was correct or not, proving the constness of
>> variables is what we have a compiler for.
>> 
>> xterm.c:5346
>> 
>> 	if (XSCROLL_BAR (bar)->x_window == window_id
>>             && FRAME_X_DISPLAY (XFRAME (frame)) == display
>> 	    && (type = 2
>> 		|| (type == 1 && XSCROLL_BAR (bar)->horizontal)
>> 		|| (type == 0 && !XSCROLL_BAR (bar)->horizontal)))
>> 	  return XSCROLL_BAR (bar);
>> 
>> Robert
> 
> Well, not everything at once! :) I afraid that if I fix lots of 
> warnings in one patch-set, it may get stuck in review because of the 
> amount of changes; besides it's easier for my sanity to send small 
> patchsets because mailing-list based projects in general tend not to 
> accept patches too quickly.
> 
> Also note: the constness here is not for compiler but for developers.
> 

bug#35062: [PATCH 0/4] Trivial code cleanups

[Robert Pluim]

>>>>> On Mon, 01 Apr 2019 16:35:50 +0300, Konstantin Kharlamov <hi-angel@yandex.ru> said:
    >> xterm.c:5346
    >> 
    >> if (XSCROLL_BAR (bar)->x_window == window_id && FRAME_X_DISPLAY
    >> (XFRAME (frame)) == display && (type = 2 || (type == 1 &&
    >> XSCROLL_BAR (bar)->horizontal) || (type == 0 && !XSCROLL_BAR
    >> (bar)->horizontal))) return XSCROLL_BAR (bar);
    >> 
    >> Robert

    Konstantin> Well, not everything at once! :) I afraid that if I
    Konstantin> fix lots of warnings in one patch-set, it may get
    Konstantin> stuck in review because of the amount of changes;
    Konstantin> besides it's easier for my sanity to send small
    Konstantin> patchsets because mailing-list based projects in
    Konstantin> general tend not to accept patches too quickly.

Thatʼs why you concentrate on things that look like errors :-)

    Konstantin> Also note: the constness here is not for compiler but
    Konstantin> for developers.

I donʼt see how developers are particularly helped in this particular
case, but then again they're not harmed either.

Robert

bug#35062: [PATCH 0/4] Trivial code cleanups

[Konstantin Kharlamov]

On Пн, Apr 1, 2019 at 15:43, Robert Pluim <rpluim@gmail.com> wrote:
>>>>>>  On Mon, 01 Apr 2019 16:35:50 +0300, Konstantin Kharlamov 
>>>>>> <hi-angel@yandex.ru> said:
>     >> xterm.c:5346
>     >>
>     >> if (XSCROLL_BAR (bar)->x_window == window_id && FRAME_X_DISPLAY
>     >> (XFRAME (frame)) == display && (type = 2 || (type == 1 &&
>     >> XSCROLL_BAR (bar)->horizontal) || (type == 0 && !XSCROLL_BAR
>     >> (bar)->horizontal))) return XSCROLL_BAR (bar);
>     >>
>     >> Robert
> 
>     Konstantin> Well, not everything at once! :) I afraid that if I
>     Konstantin> fix lots of warnings in one patch-set, it may get
>     Konstantin> stuck in review because of the amount of changes;
>     Konstantin> besides it's easier for my sanity to send small
>     Konstantin> patchsets because mailing-list based projects in
>     Konstantin> general tend not to accept patches too quickly.
> 
> Thatʼs why you concentrate on things that look like errors :-)
> 
>     Konstantin> Also note: the constness here is not for compiler but
>     Konstantin> for developers.
> 
> I donʼt see how developers are particularly helped in this particular
> case, but then again they're not harmed either.

For illustration you can take a look at patch 3/4 here. There, I 
constify min_rows and min_cols; and afterwards I remove a paragraph 
that compares them to a number that wasn't assigned there. This allows 
to not look through the code before the comparison because it's 
immediately obvious: these variables are never changed.

This is true for reading the code as well, especially when you're 
debugging a problem: you may often wonder "okay, when was the last time 
that variable changed, could it be invalid here?". Then, if it's 
"const" you immediately know the answer, whereas otherwise you have to 
search through all usages of that variable to see when was the last 
time it changed.

bug#35062: [PATCH 0/4] Trivial code cleanups

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Konstantin Kharlamov <Hi-Angel@yandex.ru>,  35062@debbugs.gnu.org
> Date: Mon, 01 Apr 2019 15:27:59 +0200
> 
> Iʼd prefer it if the effort went to determining if eg the alert for
> 'type = 2' below was correct or not

Isn't it clear?

Btw, AFAICT, that function is _always_ called with type == 2 in
xterm.c, never with any other value.  Only its other implementations
can be called with a different value of TYPE.

bug#35062: [PATCH 0/4] Trivial code cleanups

[Robert Pluim]

>>>>> On Mon, 01 Apr 2019 17:34:46 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com> Cc: Konstantin Kharlamov
    >> <Hi-Angel@yandex.ru>, 35062@debbugs.gnu.org Date: Mon, 01 Apr
    >> 2019 15:27:59 +0200
    >> 
    >> Iʼd prefer it if the effort went to determining if eg the alert
    >> for 'type = 2' below was correct or not

    Eli> Isn't it clear?

No, because you had to do your analysis below first to see if using '=='
changed the meaning of the code.

    Eli> Btw, AFAICT, that function is _always_ called with type == 2
    Eli> in xterm.c, never with any other value.  Only its other
    Eli> implementations can be called with a different value of TYPE.

BTW, the w32term.c implementation has the same typo :-)

Perhaps we can just delete most of that condition (I canʼt test at the
moment, my X11 machine is down).

diff --git a/src/xterm.c b/src/xterm.c
index f90d6713b0..a52f2c0862 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -5342,10 +5342,7 @@ x_window_to_scroll_bar (Display *display, Window window_id, int type)
 			       ! NILP (bar));
 	   bar = XSCROLL_BAR (bar)->next)
 	if (XSCROLL_BAR (bar)->x_window == window_id
-            && FRAME_X_DISPLAY (XFRAME (frame)) == display
-	    && (type = 2
-		|| (type == 1 && XSCROLL_BAR (bar)->horizontal)
-		|| (type == 0 && !XSCROLL_BAR (bar)->horizontal)))
+            && FRAME_X_DISPLAY (XFRAME (frame)) == display)
 	  return XSCROLL_BAR (bar);
     }
 

bug#35062: [PATCH 0/4] Trivial code cleanups

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 35062@debbugs.gnu.org,  Hi-Angel@yandex.ru
> Date: Mon, 01 Apr 2019 17:04:52 +0200
> 
>     Eli> Btw, AFAICT, that function is _always_ called with type == 2
>     Eli> in xterm.c, never with any other value.  Only its other
>     Eli> implementations can be called with a different value of TYPE.
> 
> BTW, the w32term.c implementation has the same typo :-)

Yes, I fixed them both right after I wrote that.

Thanks.

bug#35062: [PATCH] Remove redundant multiplication of ch and cw

[Konstantin Kharlamov]

* widget.c: multiplication by zero always equals zero
---
 src/widget.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/widget.c b/src/widget.c
index c695bd5f305..508974dd46f 100644
--- a/src/widget.c
+++ b/src/widget.c
@@ -297,7 +297,6 @@ update_wm_hints (EmacsFrame ew)
   int char_height;
   int base_width;
   int base_height;
-  int min_rows = 0, min_cols = 0;
 
   /* This happens when the frame is just created.  */
   if (! wmshell) return;
@@ -323,8 +322,8 @@ update_wm_hints (EmacsFrame ew)
 		 XtNbaseHeight, (XtArgVal) base_height,
 		 XtNwidthInc, (XtArgVal) (frame_resize_pixelwise ? 1 : cw),
 		 XtNheightInc, (XtArgVal) (frame_resize_pixelwise ? 1 : ch),
-		 XtNminWidth, (XtArgVal) (base_width + min_cols * cw),
-		 XtNminHeight, (XtArgVal) (base_height + min_rows * ch),
+		 XtNminWidth, (XtArgVal) base_width,
+		 XtNminHeight, (XtArgVal) base_height,
 		 NULL);
 }
 
-- 
2.21.0

bug#35062: [PATCH] Remove redundant multiplication of ch and cw

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Tue,  2 Apr 2019 23:49:58 +0300
> 
> * widget.c: multiplication by zero always equals zero
> ---
>  src/widget.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Thanks, pushed to the master branch (with appropriate commit log
message).

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

* src/xterm.c (x_set_frame_alpha): due to preceding checks it is bound
to be 0 ≤ alpha ≤ 1. (Bug#35062)
---
v3: mention functions changed in commit messages, mention the bug
number, and don't mention that it fixes a warning since intention  of
changes is clear either way.

 src/xterm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xterm.c b/src/xterm.c
index f90d6713b02..e8f1e576a38 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -931,7 +931,7 @@ x_set_frame_alpha (struct frame *f)
     return;
   else if (alpha > 1.0)
     alpha = 1.0;
-  else if (0.0 <= alpha && alpha < alpha_min && alpha_min <= 1.0)
+  else if (alpha < alpha_min && alpha_min <= 1.0)
     alpha = alpha_min;
 
   opac = alpha * OPAQUE;
-- 
2.21.0

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

* src/xterm.c (x_cr_draw_image, x_update_begin,
x_clear_under_internal_border, x_draw_fringe_bitmap,
x_set_glyph_string_clipping, x_draw_glyph_string_background,
x_draw_composite_glyph_string_foreground, x_send_scroll_bar_event): make
code easier to follow by making explicit that some variables are
immutable. (Bug#35062)
---
v3: mention functions changed in commit messages, mention the bug
number, and don't mention that it fixes a warning since intention  of
changes is clear either way.

 src/xterm.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/xterm.c b/src/xterm.c
index e8f1e576a38..8354ce00700 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -495,9 +495,9 @@ x_cr_draw_image (struct frame *f, GC gc, cairo_surface_t *image,
       cairo_fill_preserve (cr);
     }
 
-  int orig_image_width = cairo_image_surface_get_width (image);
+  const int orig_image_width = cairo_image_surface_get_width (image);
   if (image_width == 0) image_width = orig_image_width;
-  int orig_image_height = cairo_image_surface_get_height (image);
+  const int orig_image_height = cairo_image_surface_get_height (image);
   if (image_height == 0) image_height = orig_image_height;
 
   cairo_pattern_t *pattern = cairo_pattern_create_for_surface (image);
@@ -1006,7 +1006,7 @@ x_update_begin (struct frame *f)
       if (FRAME_GTK_WIDGET (f))
         {
           GdkWindow *w = gtk_widget_get_window (FRAME_GTK_WIDGET (f));
-	  int scale = xg_get_scale (f);
+	  const int scale = xg_get_scale (f);
 	  width = scale * gdk_window_get_width (w);
 	  height = scale * gdk_window_get_height (w);
         }
@@ -1300,15 +1300,15 @@ x_clear_under_internal_border (struct frame *f)
 {
   if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0)
     {
-      int border = FRAME_INTERNAL_BORDER_WIDTH (f);
-      int width = FRAME_PIXEL_WIDTH (f);
-      int height = FRAME_PIXEL_HEIGHT (f);
+      const int border = FRAME_INTERNAL_BORDER_WIDTH (f);
+      const int width = FRAME_PIXEL_WIDTH (f);
+      const int height = FRAME_PIXEL_HEIGHT (f);
 #ifdef USE_GTK
-      int margin = 0;
+      const int margin = 0;
 #else
-      int margin = FRAME_TOP_MARGIN_HEIGHT (f);
+      const int margin = FRAME_TOP_MARGIN_HEIGHT (f);
 #endif
-      int face_id =
+      const int face_id =
 	!NILP (Vface_remapping_alist)
 	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
 	: INTERNAL_BORDER_FACE_ID;
@@ -1455,7 +1455,7 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
       Drawable drawable = FRAME_X_DRAWABLE (f);
       char *bits;
       Pixmap pixmap, clipmask = (Pixmap) 0;
-      int depth = DefaultDepthOfScreen (FRAME_X_SCREEN (f));
+      const int depth = DefaultDepthOfScreen (FRAME_X_SCREEN (f));
       XGCValues gcv;
 
       if (p->wd > 8)
@@ -1704,7 +1704,7 @@ static void
 x_set_glyph_string_clipping (struct glyph_string *s)
 {
   XRectangle *r = s->clip;
-  int n = get_glyph_string_clip_rects (s, r, 2);
+  const int n = get_glyph_string_clip_rects (s, r, 2);
 
   if (n > 0)
     x_set_clip_rectangles (s->f, s->gc, r, n);
@@ -1797,7 +1797,7 @@ x_draw_glyph_string_background (struct glyph_string *s, bool force_p)
      shouldn't be drawn in the first place.  */
   if (!s->background_filled_p)
     {
-      int box_line_width = max (s->face->box_line_width, 0);
+      const int box_line_width = max (s->face->box_line_width, 0);
 
       if (s->stippled_p)
 	{
@@ -1908,15 +1908,15 @@ x_draw_composite_glyph_string_foreground (struct glyph_string *s)
     }
   else if (! s->first_glyph->u.cmp.automatic)
     {
-      int y = s->ybase;
+      const int y = s->ybase;
 
       for (i = 0, j = s->cmp_from; i < s->nchars; i++, j++)
 	/* TAB in a composition means display glyphs with padding
 	   space on the left or right.  */
 	if (COMPOSITION_GLYPH (s->cmp, j) != '\t')
 	  {
-	    int xx = x + s->cmp->offsets[j * 2];
-	    int yy = y - s->cmp->offsets[j * 2 + 1];
+	    const int xx = x + s->cmp->offsets[j * 2];
+	    const int yy = y - s->cmp->offsets[j * 2 + 1];
 
 	    font->driver->draw (s, j, j + 1, xx, yy, false);
 	    if (s->face->overstrike)
@@ -5516,7 +5516,7 @@ x_send_scroll_bar_event (Lisp_Object window, enum scroll_bar_part part,
   struct frame *f = XFRAME (w->frame);
   intptr_t iw = (intptr_t) w;
   verify (INTPTR_WIDTH <= 64);
-  int sign_shift = INTPTR_WIDTH - 32;
+  const int sign_shift = INTPTR_WIDTH - 32;
 
   block_input ();
 
-- 
2.21.0

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Sun,  7 Apr 2019 05:13:30 +0300
> 
> * src/xterm.c (x_cr_draw_image, x_update_begin,
> x_clear_under_internal_border, x_draw_fringe_bitmap,
> x_set_glyph_string_clipping, x_draw_glyph_string_background,
> x_draw_composite_glyph_string_foreground, x_send_scroll_bar_event): make
> code easier to follow by making explicit that some variables are
> immutable. (Bug#35062)
> ---
> v3: mention functions changed in commit messages, mention the bug
> number, and don't mention that it fixes a warning since intention  of
> changes is clear either way.

I'm really struggling with these changes.  My main problem is that I
don't see how using 'const' here improves the readability and clarity
of the code.  IMO, if the variable's name doesn't state its purpose,
adding 'const' won't help much.  And I think compilers nowadays are
smart enough to deduce this by themselves.

We had in the past similar issue with the 'register' qualifier;
nowadays we remove those whenever we change code that uses them.
Isn't it the same with 'const'?

Paul, could you please comment on these proposed changes?

Thanks.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

В Сб, апр 13, 2019 at 11:15, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>>  Date: Sun,  7 Apr 2019 05:13:30 +0300
>> 
>>  * src/xterm.c (x_cr_draw_image, x_update_begin,
>>  x_clear_under_internal_border, x_draw_fringe_bitmap,
>>  x_set_glyph_string_clipping, x_draw_glyph_string_background,
>>  x_draw_composite_glyph_string_foreground, x_send_scroll_bar_event): 
>> make
>>  code easier to follow by making explicit that some variables are
>>  immutable. (Bug#35062)
>>  ---
>>  v3: mention functions changed in commit messages, mention the bug
>>  number, and don't mention that it fixes a warning since intention  
>> of
>>  changes is clear either way.
> 
> I'm really struggling with these changes.  My main problem is that I
> don't see how using 'const' here improves the readability and clarity
> of the code.  IMO, if the variable's name doesn't state its purpose,
> adding 'const' won't help much.  And I think compilers nowadays are
> smart enough to deduce this by themselves.

I've had an example somewhere at the beginning of this thread, so let 
me quote myself

> For illustration you can take a look at patch 3/4 here. There, I
> constify min_rows and min_cols; and afterwards I remove a paragraph
> that compares them to a number that wasn't assigned there. This allows
> to not look through the code before the comparison because it's
> immediately obvious: these variables are never changed.
> 
> This is true for reading the code as well, especially when you're
> debugging a problem: you may often wonder "okay, when was the last 
> time
> that variable changed, could it be invalid here?". Then, if it's
> "const" you immediately know the answer, whereas otherwise you have to
> search through all usages of that variable to see when was the last
> time it changed.

Basically, less mutability means easier to read code. Usually, fully 
immutable code (e.g. in languages such as Haskell) keeps algorithm as 
clean as possible, whereas mutability adds points of mental burden 
(i.e. because you don't know without going through whole function 
whether variable was only assigned once, or was it changed somewhere).

Modern systems language Rust by default is immutable, and mutable 
variables has to be explicitly marked as such (unlike it older 
languages, where everything is mutable, and you have to `const`ify 
things).

-----

Either way, if you folks really in doubt, I'm okay with dropping this 
patch. It took me maybe 10 minutes to assemble it, so it's not like 
there's much work went into it. I simply found a place for improvement 
in code, and did that.

I'd like to take this as an opportunity to ask a question: I see Emacs 
C code is using the old style where variables (mostly) are declared at 
the beginning of a function. Is it just a legacy from C89 days (I hope 
so), or is it a mandatory style?

I'm asking because if I gonna work on the code, I'd for sure like to 
encapsulate variables as much as possible, which means I'd rather 
declare them on the first use (as a bonus, this often may allow to 
constify the variable too).

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Eli Zaretskii]

> Date: Sat, 13 Apr 2019 14:30:46 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: Paul Eggert <eggert@cs.ucla.edu>, 35062@debbugs.gnu.org
> 
> I'd like to take this as an opportunity to ask a question: I see Emacs 
> C code is using the old style where variables (mostly) are declared at 
> the beginning of a function. Is it just a legacy from C89 days (I hope 
> so), or is it a mandatory style?

It's legacy.  Nowadays we allow declarations near the first usage, per
C99.  But again, we only make changes in the order as part of other
code changes, not just by themselves.

> I'm asking because if I gonna work on the code, I'd for sure like to 
> encapsulate variables as much as possible, which means I'd rather 
> declare them on the first use (as a bonus, this often may allow to 
> constify the variable too).

That's okay.  We require C99 these days.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Paul Eggert]

I'd rather not use 'const' on locals. Let me tell you a story as to why.

I once worked with someone who insisted on using the 'register' keyword on every 
C local variable that was never taken the address of, on the grounds that this 
meant the reader could easily see that the variable could never be modified 
indirectly via a pointer and that this made the code easier to read because you 
didn't need to worry about aliasing.

I disagreed then, and still disagree. Saying 'register' nearly all the time 
clutters up the code, and the cost is not worth the benefit in C. It's pretty 
easy for a human reader to determine whether a local variable is taken the 
address of somewher in its function. (If it's hard, then write an Elisp function 
that will tell you. :-) In hindsight, perhaps C should have been designed so 
that 'register' was the default for local variables, and that one needed a 
special word to say "watch out! this variable might have its address taken!"; 
but the ship has sailed.

'const' is like 'register' in this respect. Putting 'const' nearly everywhere 
clutters C code. It's pretty easy for a human reader to determine whether a 
local variable is modified (if it's hard, write an Elisp function :-). In 
hindsight, perhaps C should have been designed so that 'const' was the default 
for local variables; but the ship has sailed there too.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

В Пт, апр 19, 2019 at 17:31, Paul Eggert <eggert@cs.ucla.edu> 
написал:
> I'd rather not use 'const' on locals. Let me tell you a story as to 
> why.
> 
> I once worked with someone who insisted on using the 'register' 
> keyword on every C local variable that was never taken the address 
> of, on the grounds that this meant the reader could easily see that 
> the variable could never be modified indirectly via a pointer and 
> that this made the code easier to read because you didn't need to 
> worry about aliasing.
> 
> I disagreed then, and still disagree. Saying 'register' nearly all 
> the time clutters up the code, and the cost is not worth the benefit 
> in C. It's pretty easy for a human reader to determine whether a 
> local variable is taken the address of somewher in its function. (If 
> it's hard, then write an Elisp function that will tell you. :-) In 
> hindsight, perhaps C should have been designed so that 'register' was 
> the default for local variables, and that one needed a special word 
> to say "watch out! this variable might have its address taken!"; but 
> the ship has sailed.
> 
> 'const' is like 'register' in this respect. Putting 'const' nearly 
> everywhere clutters C code. It's pretty easy for a human reader to 
> determine whether a local variable is modified.

"const" is only similar to "register" in a way of having an additional 
word. Sure it's easy for a human to find whether variable was modified, 
but this requires to go through the whole function highlighting 
variable usages. Now imagine if visibility scope has lots of variables? 
This is especially relevant for the old C89 style where every variable 
was declared at the beginning of a function.

And btw, current workflow with searching through the body doesn't work 
well in vanilla Emacs. While other editors allow you to highlight 
matches by putting a caret over a symbol, Emacs requires either 
manually making it highlight/unhighlight, or installing a separate 
package highlight-symbol.el which is a bit buggy and wasn't updated 
since 2016. That is to say, having non-modified things declared with 
"const" may help an average Emacs developer.

> (if it's hard, write an Elisp function :-)

But situation in there is similar: declaring a local variable requires 
to use (let…), which means that even if you use variable once in the 
end of a function, the variable will be visible to everything else. And 
it's not like you can can make it immutable either.

>  In hindsight, perhaps C should have been designed so that 'const' 
> was the default for local variables; but the ship has sailed there 
> too.

Yeah… FWIW, there's an "REmacs" effort on rewriting Emacs from C to 
Rust, Rust has everything "const" by default. I'm sure emacs-devel 
would largely disagree on integrating it because Rust isn't stable yet 
(despite being widely deployed) and there's no GCC backend (LLVM only), 
which are reasonable points. Still, I'm hoping at some point it will 
change :)

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

В Сб, апр 20, 2019 at 04:09, Konstantin Kharlamov 
<hi-angel@yandex.ru> написал:
> 
> 
> В Пт, апр 19, 2019 at 17:31, Paul Eggert <eggert@cs.ucla.edu> 
> написал:
>> I'd rather not use 'const' on locals. Let me tell you a story as to 
>> \x7fwhy.
>> 
>> I once worked with someone who insisted on using the 'register' 
>> \x7fkeyword on every C local variable that was never taken the address 
>> \x7fof, on the grounds that this meant the reader could easily see that 
>> \x7fthe variable could never be modified indirectly via a pointer and 
>> \x7fthat this made the code easier to read because you didn't need to 
>> \x7fworry about aliasing.
>> 
>> I disagreed then, and still disagree. Saying 'register' nearly all 
>> \x7fthe time clutters up the code, and the cost is not worth the 
>> benefit \x7fin C. It's pretty easy for a human reader to determine 
>> whether a \x7flocal variable is taken the address of somewher in its 
>> function. (If \x7fit's hard, then write an Elisp function that will 
>> tell you. :-) In \x7fhindsight, perhaps C should have been designed so 
>> that 'register' was \x7fthe default for local variables, and that one 
>> needed a special word \x7fto say "watch out! this variable might have 
>> its address taken!"; but \x7fthe ship has sailed.
>> 
>> 'const' is like 'register' in this respect. Putting 'const' nearly 
>> \x7feverywhere clutters C code. It's pretty easy for a human reader to 
>> \x7fdetermine whether a local variable is modified.
> 
> "const" is only similar to "register" in a way of having an 
> additional word. Sure it's easy for a human to find whether variable 
> was modified, but this requires to go through the whole function 
> highlighting variable usages. Now imagine if visibility scope has 
> lots of variables? This is especially relevant for the old C89 style 
> where every variable was declared at the beginning of a function.
> 
> And btw, current workflow with searching through the body doesn't 
> work well in vanilla Emacs. While other editors allow you to 
> highlight matches by putting a caret over a symbol, Emacs requires 
> either manually making it highlight/unhighlight, or installing a 
> separate package highlight-symbol.el which is a bit buggy and wasn't 
> updated since 2016. That is to say, having non-modified things 
> declared with "const" may help an average Emacs developer.

Shameless plug here: I'm using "color-identifiers" package, which 
colors variables in different colors. This is an amazing help! Still, 
unfortunately a number of immediately clear distinct colors is quite 
limited, so I'm still often use highlight-symbol.el functional.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Eli Zaretskii]

> Date: Sat, 20 Apr 2019 04:09:19 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
> 
> And btw, current workflow with searching through the body doesn't work 
> well in vanilla Emacs. While other editors allow you to highlight 
> matches by putting a caret over a symbol, Emacs requires either 
> manually making it highlight/unhighlight, or installing a separate 
> package highlight-symbol.el which is a bit buggy and wasn't updated 
> since 2016. That is to say, having non-modified things declared with 
> "const" may help an average Emacs developer.

I don't think I follow: Emacs's search commands do highlight the
matches, and we also have symbol-search commands ("M-s _" etc.), so
what exactly is missing?

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

On Сб, Apr 20, 2019 at 09:53, Eli Zaretskii <eliz@gnu.org> wrote:
>>  Date: Sat, 20 Apr 2019 04:09:19 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
>> 
>>  And btw, current workflow with searching through the body doesn't 
>> work
>>  well in vanilla Emacs. While other editors allow you to highlight
>>  matches by putting a caret over a symbol, Emacs requires either
>>  manually making it highlight/unhighlight, or installing a separate
>>  package highlight-symbol.el which is a bit buggy and wasn't updated
>>  since 2016. That is to say, having non-modified things declared with
>>  "const" may help an average Emacs developer.
> 
> I don't think I follow: Emacs's search commands do highlight the
> matches, and we also have symbol-search commands ("M-s _" etc.), so
> what exactly is missing?

See: to any search a word in Emacs you have to either type it manually 
or copy-paste it. And then if you get unlucky to have next match 
offscreen, the search gonna carry currently visible portion of text 
away.

In other editors and IDEs it's implemented instead by selecting a word, 
which makes the editor to highlight matches. That's similar to what 
highlight-symbol.el is doing: you put a caret over a text, and after a 
short timeout (IIRC you can't set timeout to 0 as it introduces lags to 
Emacs) it highlights visible matches of the symbol under the caret. And 
then, if you want to, you can press a hotkey to lock the highlight.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Eli Zaretskii]

> Date: Sat, 20 Apr 2019 13:31:44 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: eggert@cs.ucla.edu, 35062@debbugs.gnu.org
> 
> On Сб, Apr 20, 2019 at 09:53, Eli Zaretskii <eliz@gnu.org> wrote:
> >>  Date: Sat, 20 Apr 2019 04:09:19 +0300
> >>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
> >>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
> >> 
> > I don't think I follow: Emacs's search commands do highlight the
> > matches, and we also have symbol-search commands ("M-s _" etc.), so
> > what exactly is missing?
> 
> See: to any search a word in Emacs you have to either type it manually 
> or copy-paste it. And then if you get unlucky to have next match 
> offscreen, the search gonna carry currently visible portion of text 
> away.
> 
> In other editors and IDEs it's implemented instead by selecting a word, 
> which makes the editor to highlight matches.

"M-s ." will highlight matches for the symbol at point, without even
requiring you to select that symbol.

> That's similar to what highlight-symbol.el is doing: you put a caret
> over a text, and after a short timeout (IIRC you can't set timeout
> to 0 as it introduces lags to Emacs) it highlights visible matches
> of the symbol under the caret. And then, if you want to, you can
> press a hotkey to lock the highlight.

So you want to avoid typing "M-s .", is that right?  But selecting a
symbol will need more keystrokes (except for very short symbols), so
where's the gain in that?

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

On Сб, Apr 20, 2019 at 14:01, Eli Zaretskii <eliz@gnu.org> wrote:
>>  Date: Sat, 20 Apr 2019 13:31:44 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: eggert@cs.ucla.edu, 35062@debbugs.gnu.org
>> 
>>  On Сб, Apr 20, 2019 at 09:53, Eli Zaretskii <eliz@gnu.org> wrote:
>>  >>  Date: Sat, 20 Apr 2019 04:09:19 +0300
>>  >>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  >>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
>>  >>
>>  > I don't think I follow: Emacs's search commands do highlight the
>>  > matches, and we also have symbol-search commands ("M-s _" etc.), 
>> so
>>  > what exactly is missing?
>> 
>>  See: to any search a word in Emacs you have to either type it 
>> manually
>>  or copy-paste it. And then if you get unlucky to have next match
>>  offscreen, the search gonna carry currently visible portion of text
>>  away.
>> 
>>  In other editors and IDEs it's implemented instead by selecting a 
>> word,
>>  which makes the editor to highlight matches.
> 
> "M-s ." will highlight matches for the symbol at point, without even
> requiring you to select that symbol.

Wow, that's cool, I didn't know about (isearch-forward-symbol-at-point).

>>  That's similar to what highlight-symbol.el is doing: you put a caret
>>  over a text, and after a short timeout (IIRC you can't set timeout
>>  to 0 as it introduces lags to Emacs) it highlights visible matches
>>  of the symbol under the caret. And then, if you want to, you can
>>  press a hotkey to lock the highlight.
> 
> So you want to avoid typing "M-s .", is that right?  But selecting a
> symbol will need more keystrokes (except for very short symbols), so
> where's the gain in that?

Well, me not knowing about "M-s ." aside, I understand one can't 
directly compare having to select a text in other editors with Emacs, 
because those are mouse-oriented, and Emacs is keyboard-oriented. The 
"Emacsy" solution is implemented in highlight-symbol.el, and it is 
quicker than typing "M-s .": you just put a caret over a symbol, and 
matches are automatically shown.

I just thought if I can write an analog of highlight-symbol.el by 
binding the "M-s ."

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

On Сб, Apr 20, 2019 at 14:23, Konstantin Kharlamov 
<hi-angel@yandex.ru> wrote:
> 
> 
> On Сб, Apr 20, 2019 at 14:01, Eli Zaretskii <eliz@gnu.org> wrote:
>>>  Date: Sat, 20 Apr 2019 13:31:44 +0300
>>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>>  Cc: eggert@cs.ucla.edu, 35062@debbugs.gnu.org
>>> 
>>>  On Сб, Apr 20, 2019 at 09:53, Eli Zaretskii <eliz@gnu.org> wrote:
>>>  >>  Date: Sat, 20 Apr 2019 04:09:19 +0300
>>>  >>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>>  >>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
>>>  >>
>>>  > I don't think I follow: Emacs's search commands do highlight the
>>>  > matches, and we also have symbol-search commands ("M-s _" etc.), 
>>> \x7f\x7fso
>>>  > what exactly is missing?
>>> 
>>>  See: to any search a word in Emacs you have to either type it 
>>> \x7f\x7fmanually
>>>  or copy-paste it. And then if you get unlucky to have next match
>>>  offscreen, the search gonna carry currently visible portion of text
>>>  away.
>>> 
>>>  In other editors and IDEs it's implemented instead by selecting a 
>>> \x7f\x7fword,
>>>  which makes the editor to highlight matches.
>> 
>> "M-s ." will highlight matches for the symbol at point, without even
>> requiring you to select that symbol.
> 
> Wow, that's cool, I didn't know about 
> (isearch-forward-symbol-at-point).
> 
>>>  That's similar to what highlight-symbol.el is doing: you put a 
>>> caret
>>>  over a text, and after a short timeout (IIRC you can't set timeout
>>>  to 0 as it introduces lags to Emacs) it highlights visible matches
>>>  of the symbol under the caret. And then, if you want to, you can
>>>  press a hotkey to lock the highlight.
>> 
>> So you want to avoid typing "M-s .", is that right?  But selecting a
>> symbol will need more keystrokes (except for very short symbols), so
>> where's the gain in that?
> 
> Well, me not knowing about "M-s ." aside, I understand one can't 
> directly compare having to select a text in other editors with Emacs, 
> because those are mouse-oriented, and Emacs is keyboard-oriented. The 
> "Emacsy" solution is implemented in highlight-symbol.el, and it is 
> quicker than typing "M-s .": you just put a caret over a symbol, and 
> matches are automatically shown.
> 
> I just thought if I can write an analog of highlight-symbol.el by 
> binding the "M-s ."

…by binding the function to idle-timeout and post that as an answer 
here 
https://stackoverflow.com/questions/385661/how-to-highlight-all-occurrences-of-a-word-in-an-emacs-buffer 
but I figured that this gonna make caret to jump to beginning of a word 
every time, so probably no.

(sorry, accidentally pressed Ctrl+enter in mail client instead of 
Ctrl+Backspace)

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

On Сб, Apr 20, 2019 at 14:25, Konstantin Kharlamov 
<hi-angel@yandex.ru> wrote:
> 
> 
> On Сб, Apr 20, 2019 at 14:23, Konstantin Kharlamov 
> <hi-angel@yandex.ru> wrote:
>> 
>> 
>> On Сб, Apr 20, 2019 at 14:01, Eli Zaretskii <eliz@gnu.org> wrote:
>>>>  Date: Sat, 20 Apr 2019 13:31:44 +0300
>>>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>>>  Cc: eggert@cs.ucla.edu, 35062@debbugs.gnu.org
>>>> 
>>>>  On Сб, Apr 20, 2019 at 09:53, Eli Zaretskii <eliz@gnu.org> 
>>>> wrote:
>>>>  >>  Date: Sat, 20 Apr 2019 04:09:19 +0300
>>>>  >>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>>>  >>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
>>>>  >>
>>>>  > I don't think I follow: Emacs's search commands do highlight the
>>>>  > matches, and we also have symbol-search commands ("M-s _" 
>>>> etc.), \x7f\x7f\x7f\x7f\x7fso
>>>>  > what exactly is missing?
>>>> 
>>>>  See: to any search a word in Emacs you have to either type it 
>>>> \x7f\x7f\x7f\x7f\x7fmanually
>>>>  or copy-paste it. And then if you get unlucky to have next match
>>>>  offscreen, the search gonna carry currently visible portion of 
>>>> text
>>>>  away.
>>>> 
>>>>  In other editors and IDEs it's implemented instead by selecting a 
>>>> \x7f\x7f\x7f\x7f\x7fword,
>>>>  which makes the editor to highlight matches.
>>> 
>>> "M-s ." will highlight matches for the symbol at point, without even
>>> requiring you to select that symbol.
>> 
>> Wow, that's cool, I didn't know about 
>> \x7f(isearch-forward-symbol-at-point).
>> 
>>>>  That's similar to what highlight-symbol.el is doing: you put a 
>>>> \x7f\x7f\x7fcaret
>>>>  over a text, and after a short timeout (IIRC you can't set timeout
>>>>  to 0 as it introduces lags to Emacs) it highlights visible matches
>>>>  of the symbol under the caret. And then, if you want to, you can
>>>>  press a hotkey to lock the highlight.
>>> 
>>> So you want to avoid typing "M-s .", is that right?  But selecting a
>>> symbol will need more keystrokes (except for very short symbols), so
>>> where's the gain in that?
>> 
>> Well, me not knowing about "M-s ." aside, I understand one can't 
>> \x7fdirectly compare having to select a text in other editors with 
>> Emacs, \x7fbecause those are mouse-oriented, and Emacs is 
>> keyboard-oriented. The \x7f"Emacsy" solution is implemented in 
>> highlight-symbol.el, and it is \x7fquicker than typing "M-s .": you 
>> just put a caret over a symbol, and \x7fmatches are automatically shown.
>> 
>> I just thought if I can write an analog of highlight-symbol.el by 
>> \x7fbinding the "M-s ."
> 
> …by binding the function to idle-timeout and post that as an answer 
> here 
> https://stackoverflow.com/questions/385661/how-to-highlight-all-occurrences-of-a-word-in-an-emacs-buffer 
> but I figured that this gonna make caret to jump to beginning of a 
> word every time, so probably no.
> 
> (sorry, accidentally pressed Ctrl+enter in mail client instead of 
> Ctrl+Backspace)
> 
Btw, I just tried setting (setq highlight-symbol-idle-delay 0) in 
highlight-symbol.el, and it doesn't lag for me! This is amazing! I'll 
try to keep it for today to see how it goes.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Konstantin Kharlamov]

On Сб, Apr 20, 2019 at 14:47, Konstantin Kharlamov 
<hi-angel@yandex.ru> wrote:
> 
> 
> On Сб, Apr 20, 2019 at 14:25, Konstantin Kharlamov 
> <hi-angel@yandex.ru> wrote:
>> 
>> 
>> On Сб, Apr 20, 2019 at 14:23, Konstantin Kharlamov 
>> \x7f<hi-angel@yandex.ru> wrote:
>>> 
>>> 
>>> On Сб, Apr 20, 2019 at 14:01, Eli Zaretskii <eliz@gnu.org> wrote:
>>>>>  Date: Sat, 20 Apr 2019 13:31:44 +0300
>>>>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>>>>  Cc: eggert@cs.ucla.edu, 35062@debbugs.gnu.org
>>>>> 
>>>>>  On Сб, Apr 20, 2019 at 09:53, Eli Zaretskii <eliz@gnu.org> 
>>>>> \x7f\x7f\x7f\x7fwrote:
>>>>>  >>  Date: Sat, 20 Apr 2019 04:09:19 +0300
>>>>>  >>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>>>>  >>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
>>>>>  >>
>>>>>  > I don't think I follow: Emacs's search commands do highlight 
>>>>> the
>>>>>  > matches, and we also have symbol-search commands ("M-s _" 
>>>>> \x7f\x7f\x7f\x7fetc.), \x7f\x7f\x7f\x7f\x7fso
>>>>>  > what exactly is missing?
>>>>> 
>>>>>  See: to any search a word in Emacs you have to either type it 
>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fmanually
>>>>>  or copy-paste it. And then if you get unlucky to have next match
>>>>>  offscreen, the search gonna carry currently visible portion of 
>>>>> \x7f\x7f\x7f\x7ftext
>>>>>  away.
>>>>> 
>>>>>  In other editors and IDEs it's implemented instead by selecting 
>>>>> a \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fword,
>>>>>  which makes the editor to highlight matches.
>>>> 
>>>> "M-s ." will highlight matches for the symbol at point, without 
>>>> even
>>>> requiring you to select that symbol.
>>> 
>>> Wow, that's cool, I didn't know about 
>>> \x7f\x7f\x7f(isearch-forward-symbol-at-point).
>>> 
>>>>>  That's similar to what highlight-symbol.el is doing: you put a 
>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7fcaret
>>>>>  over a text, and after a short timeout (IIRC you can't set 
>>>>> timeout
>>>>>  to 0 as it introduces lags to Emacs) it highlights visible 
>>>>> matches
>>>>>  of the symbol under the caret. And then, if you want to, you can
>>>>>  press a hotkey to lock the highlight.
>>>> 
>>>> So you want to avoid typing "M-s .", is that right?  But selecting 
>>>> a
>>>> symbol will need more keystrokes (except for very short symbols), 
>>>> so
>>>> where's the gain in that?
>>> 
>>> Well, me not knowing about "M-s ." aside, I understand one can't 
>>> \x7f\x7f\x7fdirectly compare having to select a text in other editors with 
>>> \x7f\x7fEmacs, \x7fbecause those are mouse-oriented, and Emacs is 
>>> \x7f\x7fkeyboard-oriented. The \x7f"Emacsy" solution is implemented in 
>>> \x7f\x7fhighlight-symbol.el, and it is \x7fquicker than typing "M-s .": you 
>>> \x7f\x7fjust put a caret over a symbol, and \x7fmatches are automatically 
>>> shown.
>>> 
>>> I just thought if I can write an analog of highlight-symbol.el by 
>>> \x7f\x7f\x7fbinding the "M-s ."
>> 
>> …by binding the function to idle-timeout and post that as an 
>> answer \x7fhere 
>> \x7fLINKIFYHeceGadbHJFGdDEFBbaGDIHJDcCEGeIJDBBcEAEd 
>> \x7fbut I figured that this gonna make caret to jump to beginning of a 
>> \x7fword every time, so probably no.
>> 
>> (sorry, accidentally pressed Ctrl+enter in mail client instead of 
>> \x7fCtrl+Backspace)
>> 
> Btw, I just tried setting (setq highlight-symbol-idle-delay 0) in 
> highlight-symbol.el, and it doesn't lag for me! This is amazing! I'll 
> try to keep it for today to see how it goes.
> 

Ah, never mind, typing the text is impossible.

bug#35062: [PATCH v3 2/3] constify a bit of xterm.c

[Eli Zaretskii]

> Cc: 35062@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 19 Apr 2019 17:31:38 -0700
> 
> 'const' is like 'register' in this respect. Putting 'const' nearly everywhere 
> clutters C code. It's pretty easy for a human reader to determine whether a 
> local variable is modified (if it's hard, write an Elisp function :-). In 
> hindsight, perhaps C should have been designed so that 'const' was the default 
> for local variables; but the ship has sailed there too.

Thanks.  That was more or less my thinking as well.

bug#35062: [PATCH v3 3/3] don't compare unsigned to less-than-zero

[Konstantin Kharlamov]

* pdumper.c (dump_anonymous_allocate, dump_map_file): don't compare
unsigned to less-than-zero (Bug#35062)
---
v3: mention functions changed in commit messages, mention the bug
number, and don't mention that it fixes a warning since intention  of
changes is clear either way.

 src/pdumper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index a9b3732a2d4..5407154fb2d 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -4434,7 +4434,6 @@ dump_anonymous_allocate (void *base,
 static void
 dump_anonymous_release (void *addr, size_t size)
 {
-  eassert (size >= 0);
 #if VM_SUPPORTED == VM_MS_WINDOWS
   (void) size;
   if (!VirtualFree (addr, 0, MEM_RELEASE))
@@ -4584,7 +4583,6 @@ dump_map_file (void *base, int fd, off_t offset, size_t size,
 static void
 dump_unmap_file (void *addr, size_t size)
 {
-  eassert (size >= 0);
 #if !VM_SUPPORTED
   (void) addr;
   (void) size;
-- 
2.21.0

bug#35062: [PATCH v3 3/3] don't compare unsigned to less-than-zero

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Sun,  7 Apr 2019 05:13:31 +0300
> 
> * pdumper.c (dump_anonymous_allocate, dump_map_file): don't compare
> unsigned to less-than-zero (Bug#35062)

A minor nit: the text after the function names should start with a
capital letter ("Don't compare..."), as any English sentence would.

> ---
> v3: mention functions changed in commit messages, mention the bug
> number, and don't mention that it fixes a warning since intention  of
> changes is clear either way.

It is true that these assertions may look redundant, but pdumper.c
uses an unusually high level of safety features, so I'd like to have a
second opinion from its author.

Daniel, are you okay with removing these assertions?

>  src/pdumper.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/pdumper.c b/src/pdumper.c
> index a9b3732a2d4..5407154fb2d 100644
> --- a/src/pdumper.c
> +++ b/src/pdumper.c
> @@ -4434,7 +4434,6 @@ dump_anonymous_allocate (void *base,
>  static void
>  dump_anonymous_release (void *addr, size_t size)
>  {
> -  eassert (size >= 0);
>  #if VM_SUPPORTED == VM_MS_WINDOWS
>    (void) size;
>    if (!VirtualFree (addr, 0, MEM_RELEASE))
> @@ -4584,7 +4583,6 @@ dump_map_file (void *base, int fd, off_t offset, size_t size,
>  static void
>  dump_unmap_file (void *addr, size_t size)
>  {
> -  eassert (size >= 0);
>  #if !VM_SUPPORTED
>    (void) addr;
>    (void) size;
> -- 
> 2.21.0

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Sun,  7 Apr 2019 05:13:29 +0300
> 
> * src/xterm.c (x_set_frame_alpha): due to preceding checks it is bound
> to be 0 ≤ alpha ≤ 1. (Bug#35062)
> ---
> v3: mention functions changed in commit messages, mention the bug
> number, and don't mention that it fixes a warning since intention  of
> changes is clear either way.

Thanks, I pushed a slightly different change along the same lines.

A minor nit: please avoid using non-ASCII characters in log entries,
if that can be avoided, to cater to terminals that don't support UTF-8
(the character you used is not in Latin-1).

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

В Сб, апр 13, 2019 at 11:06, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>>  Date: Sun,  7 Apr 2019 05:13:29 +0300
>> 
>>  * src/xterm.c (x_set_frame_alpha): due to preceding checks it is 
>> bound
>>  to be 0 ≤ alpha ≤ 1. (Bug#35062)
>>  ---
>>  v3: mention functions changed in commit messages, mention the bug
>>  number, and don't mention that it fixes a warning since intention  
>> of
>>  changes is clear either way.
> 
> Thanks, I pushed a slightly different change along the same lines.
> 
> A minor nit: please avoid using non-ASCII characters in log entries,
> if that can be avoided, to cater to terminals that don't support UTF-8
> (the character you used is not in Latin-1).

Err… I just looked at log — wait, what?? There is no my patch. You 
crafted a patch of your own, incorporated my changes, and pushed as 
your own. If I gonna pass someone this link 
http://git.savannah.gnu.org/cgit/emacs.git/log/?qt=author&q=Konstantin 
there is no my contribution.

More over: you didn't even tell me! Wow! This is not good.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> Date: Sat, 13 Apr 2019 21:19:12 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: 35062@debbugs.gnu.org
> 
> Err… I just looked at log — wait, what?? There is no my patch. You 
> crafted a patch of your own, incorporated my changes, and pushed as 
> your own. If I gonna pass someone this link 
> http://git.savannah.gnu.org/cgit/emacs.git/log/?qt=author&q=Konstantin 
> there is no my contribution.
> 
> More over: you didn't even tell me! Wow! This is not good.

What do you mean?  Your name is in the log.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

В Сб, апр 13, 2019 at 21:24, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  Date: Sat, 13 Apr 2019 21:19:12 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: 35062@debbugs.gnu.org
>> 
>>  Err… I just looked at log — wait, what?? There is no my patch. 
>> You
>>  crafted a patch of your own, incorporated my changes, and pushed as
>>  your own. If I gonna pass someone this link
>>  
>> http://git.savannah.gnu.org/cgit/emacs.git/log/?qt=author&q=Konstantin
>>  there is no my contribution.
>> 
>>  More over: you didn't even tell me! Wow! This is not good.
> 
> What do you mean?  Your name is in the log.

If you click the link above you won't find it. The only mention of my 
name is in log message as "suggested-by". Which btw sounds like I 
reported a bug "hey guys, I don't know how to code, but static analyzer 
says this part can be improved, can anybody do it for me?".

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> Date: Sat, 13 Apr 2019 21:28:39 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: 35062@debbugs.gnu.org
> 
> >> http://git.savannah.gnu.org/cgit/emacs.git/log/?qt=author&q=Konstantin
> >>  there is no my contribution.
> >> 
> >>  More over: you didn't even tell me! Wow! This is not good.
> > 
> > What do you mean?  Your name is in the log.
> 
> If you click the link above you won't find it. The only mention of my 
> name is in log message as "suggested-by".

If you are looking at partial information, don't be surprised that you
see only some part of what was done and written.  Emacs contributors
are generally expected to have a clone of the repository on their
local machine.  Don't you have it?

> Which btw sounds like I reported a bug "hey guys, I don't know how
> to code, but static analyzer says this part can be improved, can
> anybody do it for me?".

That's not what it means.  "Suggested by" means you suggested a code
change, but I tweaked it enough to feel that you can no longer be held
responsible for whatever mistakes I might have made in the actual code
change I committed.  And I explicitly said that this is what I did: "I
pushed a slightly different change along the same lines."  There's
also a reference to the bug number, where what you submitted is
clearly visible.  The "I don't know how to code" variant would have
been identified as "reported by" instead.

This is all standard practice around here (and in other projects as
well).

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  >   "Suggested by" means you suggested a code
  > change, but I tweaked it enough to feel that you can no longer be held
  > responsible for whatever mistakes I might have made in the actual code
  > change I committed.

I think you're saying that "suggested by" means that the code changes
were suggested by that person -- not merely that the idea was
suggested by per.  Is that it?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

On Вс, Apr 14, 2019 at 23:38, Richard Stallman <rms@gnu.org> wrote:
> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
> 
>   >   "Suggested by" means you suggested a code
>   > change, but I tweaked it enough to feel that you can no longer be 
> held
>   > responsible for whatever mistakes I might have made in the actual 
> code
>   > change I committed.
> 
> I think you're saying that "suggested by" means that the code changes
> were suggested by that person -- not merely that the idea was
> suggested by per.  Is that it?

That's what you quoted :)

IMO in such situations (i.e. when original changes were commited 
without any modification anyway) would be nice to commit the original 
patch, and then add up further improvements as 2-nd commit. Ultimately 
what makes me sad is that if I'd want to refer to my commits in Emacs 
as part of a CV, it's hard to find all suggested-by and authored 
commits at the same time, and also that suggested-by sounds kind of 
vague to have an influence in CV.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> Date: Mon, 15 Apr 2019 09:49:08 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
> 
> IMO in such situations (i.e. when original changes were commited 
> without any modification anyway) would be nice to commit the original 
> patch, and then add up further improvements as 2-nd commit.

That's true, but this is not such a situation: the original changes
were never committed without any modifications.

Sometimes committing the original and then making changes in a
followup is TRT, and sometimes it isn't; it's a judgment call.  In
general, the decision depends on the percentage of the original
submission that the committer would like to change, and also on the
overall volume of the original submission.  In this case, the original
patch was relatively small, and I modified it in relatively
significant ways.  So it made little sense to commit something that
would be immediately modified in significant ways, it would just be
extra work for no good reason.

> Ultimately what makes me sad is that if I'd want to refer to my
> commits in Emacs as part of a CV, it's hard to find all suggested-by
> and authored commits at the same time, and also that suggested-by
> sounds kind of vague to have an influence in CV.

You can always use "git log --grep" to find references to your
contributions in the log messages.  And the log message includes a
reference to the bug number, where you can refer people for your
actual contribution.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

On Пн, Apr 15, 2019 at 17:32, Eli Zaretskii <eliz@gnu.org> wrote:
>>  Date: Mon, 15 Apr 2019 09:49:08 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: Eli Zaretskii <eliz@gnu.org>, 35062@debbugs.gnu.org
>> 
>>  IMO in such situations (i.e. when original changes were commited
>>  without any modification anyway) would be nice to commit the 
>> original
>>  patch, and then add up further improvements as 2-nd commit.
> 
> That's true, but this is not such a situation: the original changes
> were never committed without any modifications.

Well, given the line my patch modifies has no changes, the only 
modification was the commit message. My only mistake was not knowing 
that UTF8 is prohibited. But really, it's a 2 symbols text replacement, 
me or you could just replace it.

> Sometimes committing the original and then making changes in a
> followup is TRT, and sometimes it isn't; it's a judgment call.  In
> general, the decision depends on the percentage of the original
> submission that the committer would like to change, and also on the
> overall volume of the original submission.  In this case, the original
> patch was relatively small, and I modified it in relatively
> significant ways.  So it made little sense to commit something that
> would be immediately modified in significant ways, it would just be
> extra work for no good reason.

Is there an extra work? The changes you added can be commited with α) 
git commit --amend -v, or β) git commit -v. You did α, which only 
differs from β by a number of characters, that is ironically smaller 
in β.

>>  Ultimately what makes me sad is that if I'd want to refer to my
>>  commits in Emacs as part of a CV, it's hard to find all suggested-by
>>  and authored commits at the same time, and also that suggested-by
>>  sounds kind of vague to have an influence in CV.
> 
> You can always use "git log --grep" to find references to your
> contributions in the log messages.  And the log message includes a
> reference to the bug number, where you can refer people for your
> actual contribution.

Who would attach a bunch of commit messages to a CV? It's unreliable 
(interviewer gotta check that these indeed are mine), and also they 
gonna bloat CV for no reason. A better way would be providing a 
web-link to a repository with commits.

----

Sorry, I actually feel embarassed that I discussing a trivial one-liner 
patch :D But I can't stop thinking that this could've happened with a 
non-one-line or maybe one-line but non-trivial contribution…

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> Date: Mon, 15 Apr 2019 18:01:35 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: rms@gnu.org, 35062@debbugs.gnu.org
> 
> > That's true, but this is not such a situation: the original changes
> > were never committed without any modifications.
> 
> Well, given the line my patch modifies has no changes, the only 
> modification was the commit message. My only mistake was not knowing 
> that UTF8 is prohibited. But really, it's a 2 symbols text replacement, 
> me or you could just replace it.

No, the log message was not the problem.  Look at the code changes,
they were the ones I modified.

> Is there an extra work? The changes you added can be commited with α) 
> git commit --amend -v, or β) git commit -v. You did α, which only 
> differs from β by a number of characters, that is ironically smaller 
> in β.

Yes, this is extra work: it requires one more commit.  More steps,
more opportunities to make mistakes, etc.  And that's if I'm not
interrupted in the middle of it by something in Real Life, or someone
pushes to upstream in-between, and I need to pull again and perhaps
resolve conflicts.  I'd rather avoid such complications for a simple
job like that.

> > You can always use "git log --grep" to find references to your
> > contributions in the log messages.  And the log message includes a
> > reference to the bug number, where you can refer people for your
> > actual contribution.
> 
> Who would attach a bunch of commit messages to a CV?

I don't know.  When I interview software engineers, I don't ask them
for such details, I can look up their contributions myself, given just
the repository URL.

> Sorry, I actually feel embarassed that I discussing a trivial one-liner 
> patch :D But I can't stop thinking that this could've happened with a 
> non-one-line or maybe one-line but non-trivial contribution…

We are splitting hair, for sure.  I think you are unfamiliar with our
procedures, and try too hard to find aspects that you saw elsewhere.
If so, it's a temporary difficulty.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

В Пн, апр 15, 2019 at 18:21, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  Date: Mon, 15 Apr 2019 18:01:35 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: rms@gnu.org, 35062@debbugs.gnu.org
>> 
>>  > That's true, but this is not such a situation: the original 
>> changes
>>  > were never committed without any modifications.
>> 
>>  Well, given the line my patch modifies has no changes, the only
>>  modification was the commit message. My only mistake was not knowing
>>  that UTF8 is prohibited. But really, it's a 2 symbols text 
>> replacement,
>>  me or you could just replace it.
> 
> No, the log message was not the problem.  Look at the code changes,
> they were the ones I modified.

The line that I changed is exactly the same. What you did is cleaning 
up the function even further, however that didn't touch my 
modifications. Your modification is not strictly relevant to my patch.

>>  Is there an extra work? The changes you added can be commited with 
>> α)
>>  git commit --amend -v, or β) git commit -v. You did α, which only
>>  differs from β by a number of characters, that is ironically 
>> smaller
>>  in β.
> 
> Yes, this is extra work: it requires one more commit.  More steps,
> more opportunities to make mistakes, etc.  And that's if I'm not
> interrupted in the middle of it by something in Real Life, or someone
> pushes to upstream in-between, and I need to pull again and perhaps
> resolve conflicts.  I'd rather avoid such complications for a simple
> job like that.

Okay, let's compare:

1. Doing extra commit:  "git commit"
2. Changing last commit "git commit --amend"

You did 2, do you see how 1 has less symbols?

What other extra steps did you take, writing the commit message? But 
since you rewrote mine anyway, this step you would take for either of 1 
and 2.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> Date: Mon, 15 Apr 2019 20:03:05 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: rms@gnu.org, 35062@debbugs.gnu.org
> 
> > No, the log message was not the problem.  Look at the code changes,
> > they were the ones I modified.
> 
> The line that I changed is exactly the same. What you did is cleaning 
> up the function even further, however that didn't touch my 
> modifications. Your modification is not strictly relevant to my patch.

That's your perspective, but not mine.  My perspective is that you
changed one line, whereas I changed 3, and by that also changed the
overall logic of the tests.

> Okay, let's compare:
> 
> 1. Doing extra commit:  "git commit"
> 2. Changing last commit "git commit --amend"
> 
> You did 2, do you see how 1 has less symbols?

No I didn't do 2.  I just edited the sources and did 1.  I never
applied your patch.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Konstantin Kharlamov]

В Пн, апр 15, 2019 at 20:16, Eli Zaretskii <eliz@gnu.org> 
написал:
>>  Date: Mon, 15 Apr 2019 20:03:05 +0300
>>  From: Konstantin Kharlamov <hi-angel@yandex.ru>
>>  Cc: rms@gnu.org, 35062@debbugs.gnu.org
>> 
>>  > No, the log message was not the problem.  Look at the code 
>> changes,
>>  > they were the ones I modified.
>> 
>>  The line that I changed is exactly the same. What you did is 
>> cleaning
>>  up the function even further, however that didn't touch my
>>  modifications. Your modification is not strictly relevant to my 
>> patch.
> 
> That's your perspective, but not mine.  My perspective is that you
> changed one line, whereas I changed 3, and by that also changed the
> overall logic of the tests.

Right, does that contradict to my statement? Your changes and mine are 
independent entities that can exist one without the other.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> Date: Mon, 15 Apr 2019 20:29:03 +0300
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Cc: rms@gnu.org, 35062@debbugs.gnu.org
> 
> > That's your perspective, but not mine.  My perspective is that you
> > changed one line, whereas I changed 3, and by that also changed the
> > overall logic of the tests.
> 
> Right, does that contradict to my statement? Your changes and mine are 
> independent entities that can exist one without the other.

In retrospect, yes.  But that's just a coincidence, because I produced
the actual patch I pushed by considering the original logic and how I
would like to change it.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > IMO in such situations (i.e. when original changes were commited 
  > without any modification anyway) would be nice to commit the original 
  > patch, and then add up further improvements as 2-nd commit.

That seems ok to me.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Mon, 15 Apr 2019 14:14:48 -0400
> Cc: 35062@debbugs.gnu.org
> 
>   > IMO in such situations (i.e. when original changes were commited 
>   > without any modification anyway) would be nice to commit the original 
>   > patch, and then add up further improvements as 2-nd commit.
> 
> That seems ok to me.

I guess you haven't yet read my response where I explained why it
wasn't OK in this case.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: hi-angel@yandex.ru, 35062@debbugs.gnu.org
> Date: Sun, 14 Apr 2019 23:38:51 -0400
> 
>   >   "Suggested by" means you suggested a code
>   > change, but I tweaked it enough to feel that you can no longer be held
>   > responsible for whatever mistakes I might have made in the actual code
>   > change I committed.
> 
> I think you're saying that "suggested by" means that the code changes
> were suggested by that person -- not merely that the idea was
> suggested by per.  Is that it?

It could mean either one, actually.

We have only a small number of attributions that change-log-mode
highlights: "Reported by", "Suggested by", and "Patches by" (each one
with a couple of variants).  So "Suggested by" will have to serve both
when someone just describes an idea and when they submit code from
which only the main ideas are taken.

If someone wants to know what exactly was "suggested", they need to
read the relevant discussions.  In this case, the log message
references a bug number, and there one can see the actual submission.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > We have only a small number of attributions that change-log-mode
  > highlights: "Reported by", "Suggested by", and "Patches by" (each one
  > with a couple of variants).

We can change that, right?  So let's decide which ones we would LIKE,
and make change-log-mode handle them.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: hi-angel@yandex.ru, 35062@debbugs.gnu.org
> Date: Tue, 16 Apr 2019 17:27:43 -0400
> 
>   > We have only a small number of attributions that change-log-mode
>   > highlights: "Reported by", "Suggested by", and "Patches by" (each one
>   > with a couple of variants).
> 
> We can change that, right?  So let's decide which ones we would LIKE,
> and make change-log-mode handle them.

I see no reason to change this, the existing possibilities are more
than enough.  I never had any problems with using them.

bug#35062: [PATCH v3 1/3] Remove redundant comparison

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > We can change that, right?  So let's decide which ones we would LIKE,
  > > and make change-log-mode handle them.

  > I see no reason to change this, the existing possibilities are more
  > than enough.  I never had any problems with using them.

We had one just now -- a contributor's career could be helped if we
distinguished one additional option.  Others may have the same wish.
It won't take much work, so let's be helpful for them.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#35062: Patches: trivial cleanups

[Lars Ingebrigtsen]

Konstantin Kharlamov <hi-angel@yandex.ru> writes:

> Per discussion on emacs-devel, this topic is created so that following
> patches wouldn't create separate reports.

I've skimmed this very long bug report now, and it's unclear to me
whether there's any remaining work to be done in this bug report.  A
number of patches were posted, and a number were applied, but I'm not
sure whether all that should have been applied were applied?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#35062: Patches: trivial cleanups

[Constantine Kharlamov]

Thanks. IIRC everything here was addressed, so it can be closed.

23.06.2019, 21:07, "Lars Ingebrigtsen" <larsi@gnus.org>:
> Konstantin Kharlamov <hi-angel@yandex.ru> writes:
>
>>  Per discussion on emacs-devel, this topic is created so that following
>>  patches wouldn't create separate reports.
>
> I've skimmed this very long bug report now, and it's unclear to me
> whether there's any remaining work to be done in this bug report. A
> number of patches were posted, and a number were applied, but I'm not
> sure whether all that should have been applied were applied?
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no