Fix three clang warnings in widget/gtk2/.

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: karlt)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Clang 3.2 gives five warnings in widget/gtk2/:

  widget/gtk2/nsDragService.cpp:1485:20 [-Wint-to-pointer-cast] cast to '_GdkAtom *' from smaller integer type 'guint' (aka 'unsigned int')

  widget/gtk2/nsGtkIMModule.cpp:298:31 [-Wself-assign] explicitly assigning a variable of type 'gpointer' (aka 'void *') to itself
  widget/gtk2/nsGtkIMModule.cpp:304:32 [-Wself-assign] explicitly assigning a variable of type 'gpointer' (aka 'void *') to itself

  widget/gtk2/nsSound.cpp:96:37 [-Wdeprecated-declarations] 'g_static_private_get' is deprecated
  widget/gtk2/nsSound.cpp:107:5 [-Wdeprecated-declarations] 'g_static_private_set' is deprecated
(Reporter)

Comment 1

6 years ago
Created attachment 697339 [details] [diff] [review]
Fix three clang warnings in widget/gtk2/.

This patch:

- Fixes the first warning by changing the type of the callback's argument.
  This compiles but I have no idea if it is correct and I haven't yet tested
  it.

- Fixes the second and third warnings in a simple manner.

- Doesn't fix the final two warnings, because I don't know enough about the
  glib APIs to do that.  (Suggestions are welcome!)
Attachment #697339 - Flags: review?(karlt)
(Reporter)

Updated

6 years ago
Blocks: 187528
(Reporter)

Comment 2

6 years ago
Oh, I also updated the emacs/vim modelines to match the ones in https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style.
(Assignee)

Comment 3

6 years ago
Comment on attachment 697339 [details] [diff] [review]
Fix three clang warnings in widget/gtk2/.

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Oh, I also updated the emacs/vim modelines to match the ones in
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style.

I'm not usually a fan of changing existing lines just to match the style guide, but I assume tw=80 does something useful (I'm not a vim user) so it makes sense to adjust the rest at the same time, as you have done.

(In reply to Nicholas Nethercote [:njn] from comment #1)
> - Fixes the first warning by changing the type of the callback's argument.
>   This compiles but I have no idea if it is correct and I haven't yet tested
>   it.

The signature of invisibleSourceDragDataGet() was correct
http://developer.gnome.org/gtk2/2.24/GtkWidget.html#GtkWidget-drag-data-get

Changing the interpretation of the parameters will have undefined behaviour
depending on the platform calling conventions, so this is not what we want.

Converting a GdkAtom to guint and back works because internally (in GDK's X11
port at least) GdkAtom is just converted to a guint index for lookup.
However all this is quite unnecessary.  I'll attach a patch to remove it.
Attachment #697339 - Flags: review?(karlt) → review-
(Assignee)

Comment 4

6 years ago
Created attachment 697617 [details] [diff] [review]
remove unnecessary GtkTargetEntry::info code

Should fix the first warning.

The GtkTargetEntry::info field (from where the aInfo parameter comes) is for
application use
http://developer.gnome.org/gtk2/2.24/gtk2-Selections.html#GtkTargetEntry
but here it just duplicates info already available.
GTK does not use this field except to pass it back to us.

gtk_target_list_new() converts the GtkTargetEntry::target strings to the same atoms in gtk_target_list_add_table():
http://git.gnome.org/browse/gtk+/tree/gtk/gtkselection.c?h=gtk-2-24#n489

These atoms are what gtk_target_list_find() matches against the target in the
GtkSelectionData before emitting the drag-data-get signal.
http://git.gnome.org/browse/gtk+/tree/gtk/gtkdnd.c?h=gtk-2-24#n3832
Attachment #697617 - Flags: review?(n.nethercote)
(Reporter)

Comment 5

6 years ago
Comment on attachment 697617 [details] [diff] [review]
remove unnecessary GtkTargetEntry::info code

Review of attachment 697617 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable, but I shouldn't be the one reviewing this.
Attachment #697617 - Flags: review?(n.nethercote) → feedback+
(Reporter)

Comment 6

6 years ago
Created attachment 697635 [details] [diff] [review]
Fix two -Wself-assign warnings in widget/gtk2/.

New version, minus the GdkAtom changes.

In the vim mode line:
- The |ts=4| and |sw=4| set the indent to 4.
- The |et| converts tabs to spaces.
- The |tw=80| sets the maximum line width to 80 chars.

Thanks for fixing the atom stuff!
Attachment #697635 - Flags: review?(karlt)
(Reporter)

Updated

6 years ago
Attachment #697339 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
(In reply to Nicholas Nethercote [:njn] from comment #0)
> Clang 3.2 gives [...] warnings in widget/gtk2/:

>   widget/gtk2/nsSound.cpp:96:37 [-Wdeprecated-declarations]
> 'g_static_private_get' is deprecated
>   widget/gtk2/nsSound.cpp:107:5 [-Wdeprecated-declarations]
> 'g_static_private_set' is deprecated

g_static_private_get is marked GLIB_DEPRECATED_IN_2_32_FOR(g_private_get).
I don't get the warning with gcc, even though I have 2.32 here.  I wonder why.

http://developer.gnome.org/glib/2.30/glib-Threads.html#GPrivate says
"GStaticPrivate is a better choice for most uses."

http://git.gnome.org/browse/glib/commit/glib/deprecated/gthread.h?id=3d4846d92309d001697c2827660fa41b5c63dbc4
says "GStaticPrivate has been made redundant by adding comparable capabilities to GPrivate."

http://developer.gnome.org/glib/2.32/glib-Threads.html#GPrivate says
"GPrivate is a very limited resource (as far as 128 per program, shared between all libraries). It is also not possible to destroy a GPrivate after it has been used. As such, it is only ever acceptable to use GPrivate in static scope, and even then sparingly so."

Perhaps GPrivate can be used instead of GStaticPrivate now, but
G_PRIVATE_INIT() is only available with older GLib 2.32 and newer, and we
build against older GLib.  And I assume GPrivate does not do the right thing
with older GLib.

Sounds like we should not be using GPrivate if at all possible.

This was added in http://hg.mozilla.org/mozilla-central/rev/9a6371bdb362
Perhaps Chris or Matthew might know a better way.

I guess this is a global context on the main thread and the issue is a context
possibly still playing during shutdown.
I wonder whether there is a way to wait (on shutdown) for the sound to finish
playing.
Or probably simpler and better, call ca_context_cancel to stop playing the
sounds.  Looks like all sounds have the same id so they are easy to cancel.
I wonder whether _cancel waits for the playing thread.

http://0pointer.de/lennart/projects/libcanberra/gtkdoc/libcanberra-canberra.html#ca-context-play
http://0pointer.de/lennart/projects/libcanberra/gtkdoc/libcanberra-canberra.html#ca-context-cancel
(Assignee)

Updated

6 years ago
Attachment #697635 - Flags: review?(karlt) → review+
(Assignee)

Comment 8

6 years ago
Comment on attachment 697617 [details] [diff] [review]
remove unnecessary GtkTargetEntry::info code

(In reply to Nicholas Nethercote [:njn] from comment #5)
> I shouldn't be the one reviewing this.

I'd often like to use that line.
Attachment #697617 - Flags: review?(ebassi)
(Reporter)

Updated

6 years ago
Assignee: n.nethercote → karlt
Comment on attachment 697617 [details] [diff] [review]
remove unnecessary GtkTargetEntry::info code

Review of attachment 697617 [details] [diff] [review]:
-----------------------------------------------------------------

looks generally okay to me
Attachment #697617 - Flags: review?(ebassi) → review+
Sorry, I backed out this fix and several others on inbound because it looks like one of them broke B2G tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c8d9373eba
(Assignee)

Comment 16

6 years ago
Filed bug 843288 for the remaining warnings.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.