Closed
Bug 826158
Opened 11 years ago
Closed 11 years ago
Fix three clang warnings in widget/gtk2/.
Categories
(Core :: Widget: Gtk, defect)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: n.nethercote, Assigned: karlt)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
13.31 KB,
patch
|
ebassi
:
review+
n.nethercote
:
feedback+
|
Details | Diff | Splinter Review |
3.36 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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•11 years ago
|
Blocks: buildwarning
Reporter | ||
Comment 2•11 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•11 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•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 years ago
|
Attachment #697339 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 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•11 years ago
|
Attachment #697635 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 8•11 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 | ||
Comment 9•11 years ago
|
||
My patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/af59387191b1
Whiteboard: [leave open]
Reporter | ||
Updated•11 years ago
|
Assignee: n.nethercote → karlt
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a911d160ec6b
Comment 13•11 years ago
|
||
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
Comment 14•11 years ago
|
||
Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/7abea998c5b6
Assignee | ||
Comment 16•11 years ago
|
||
Filed bug 843288 for the remaining warnings.
Status: NEW → RESOLVED
Closed: 11 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.
Description
•