bug#35062: Patches: trivial cleanups

classic Classic list List threaded Threaded
56 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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.





Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov
In reply to this post by 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




Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov
In reply to this post by 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




Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov
In reply to this post by 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




Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Konstantin Kharlamov
> From: Konstantin Kharlamov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Mon, 01 Apr 2019 07:37:23 +0300, Eli Zaretskii <[hidden email]> said:

    >> From: Konstantin Kharlamov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov


On Пн, Apr 1, 2019 at 15:27, Robert Pluim <[hidden email]> wrote:

>>>>>>  On Mon, 01 Apr 2019 07:37:23 +0300, Eli Zaretskii
>>>>>> <[hidden email]> said:
>
>     >> From: Konstantin Kharlamov <[hidden email]> 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.





Reply | Threaded
Open this post in threaded view
|

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
<[hidden email]> wrote:

>
>
> On Пн, Apr 1, 2019 at 15:27, Robert Pluim <[hidden email]> wrote:
>>>>>>>  On Mon, 01 Apr 2019 07:37:23 +0300, Eli Zaretskii
>>>>>>> <[hidden email]> said:
>>
>>     >> From: Konstantin Kharlamov <[hidden email]> 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.
>





Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
In reply to this post by Konstantin Kharlamov
>>>>> On Mon, 01 Apr 2019 16:35:50 +0300, Konstantin Kharlamov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov


On Пн, Apr 1, 2019 at 15:43, Robert Pluim <[hidden email]> wrote:

>>>>>>  On Mon, 01 Apr 2019 16:35:50 +0300, Konstantin Kharlamov
>>>>>> <[hidden email]> 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.





Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Robert Pluim
> From: Robert Pluim <[hidden email]>
> Cc: Konstantin Kharlamov <[hidden email]>,  [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Robert Pluim
>>>>> On Mon, 01 Apr 2019 17:34:46 +0300, Eli Zaretskii <[hidden email]> said:

    >> From: Robert Pluim <[hidden email]> Cc: Konstantin Kharlamov
    >> <[hidden email]>, [hidden email] 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);
     }
 



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky
In reply to this post by Konstantin Kharlamov
Konstantin Kharlamov <[hidden email]> 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)



Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
написал:

> Konstantin Kharlamov <[hidden email]> 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.





Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov
In reply to this post by Noam Postavsky
* 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




Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Konstantin Kharlamov
> Date: Tue, 02 Apr 2019 03:09:14 +0300
> From: Konstantin Kharlamov <[hidden email]>
> Cc: [hidden email]
>
> 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.)



Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov
In reply to this post by 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




123