Created attachment 134579 [details] [review] Fix warning: comparison of unsigned expression
Created attachment 134580 [details] [review] Fix warning implicit declaration of function
Created attachment 134581 [details] [review] Fix deprecated warnings in glib demo There is one deprecated warning remaining in the glib demo. Carlos, could you have a look. I don't know how to fix it.
Created attachment 134582 [details] [review] Fix some -Wundef warnings
Created attachment 134583 [details] [review] Use _WIN32 instead of WIN32
Created attachment 134584 [details] [review] Remove unused t1lib code for the obsolete t1lib library
Created attachment 134585 [details] [review] Remove freetype macros
Created attachment 134586 [details] [review] Fix remaining -Wundef warnings and make -Wundef a default warning
Created attachment 134587 [details] [review] Use cstdint and drop the stdint checks and emulation
Created attachment 134588 [details] [review] Remove HAVE_CONFIG_H macro
Created attachment 134589 [details] [review] Remove unused config.h macros
Created attachment 134592 [details] [review] HAVE_PTHREAD is not used And one more I missed.
Created attachment 134603 [details] [review] Use -pthread instead of -lpthread when compiling
Created attachment 134604 [details] [review] Make poppler compile of threads not available This makes poppler compile if no thread library is found. The 77 instances of #ifdef MULTITHREADED could be removed if we changed the GooMutex class so that it is always included but the functions are a no op if MULTITHREADED is not defined.
Created attachment 134605 [details] [review] Remove unused HAVE_ZLIB_H/HAVE_LIBZ More unused macros
Created attachment 134606 [details] [review] move macros out of poppler-config.h that don't need to be there
Created attachment 134607 [details] [review] Move strtok_r emulation to goo/glibc
Created attachment 134608 [details] [review] Use cmath and remove fmin/fmax from poppler-config.h
Created attachment 134609 [details] [review] stdio.h not needed in poppler-config.h
Created attachment 134610 [details] [review] c++ has long long
Created attachment 134611 [details] [review] c++ has long long
Created attachment 134612 [details] [review] use <cmath> for isfinite
Created attachment 134613 [details] [review] Remove VC7 workaround
Created attachment 134617 [details] [review] mingw threads fix
Created attachment 134618 [details] [review] mingw warnings fix
Created attachment 134619 [details] [review] mingw warnings fix
I'm aware of this, but will have to wait at least until tuesday/wednesday next week until i can have a look.
Created attachment 134692 [details] [review] mingw warnings fix
> Fix warning: comparison of unsigned expression Can you keep the const?
> Fix warning implicit declaration of function Going to need some more explanation of what this actually fixes.
> Fix deprecated warnings in glib demo I'll leave this for someone that knows gstuff
> Fix some -Wundef warnings I'm actually kind of scared of this patch, we kind of know "it works" as of now, and changing this is kind of fragile, but ok i guess
> Use _WIN32 instead of WIN32 I've no idea about windows, if you know this is correct go ahead, if not sure find someone else to have a look :)
> Remove unused t1lib code for the obsolete t1lib library -// or cmake: DISABLE_OUTLINE, DEBUG_MEM, SPLASH_CMYK, HAVE_T1LIB_H, +// or cmake: DISABLE_OUTLINE, DEBUG_MEM, You removed SPLASH_CMYK by mistake i guess otherwise, fine
> Remove freetype macros Ok
> Fix remaining -Wundef warnings and make -Wundef a default warning Again very scared about this, but if you're sure it's fine, go ahead.
> Use cstdint and drop the stdint checks and emulation Ok
> Remove HAVE_CONFIG_H macro Ok
> Remove unused config.h macros the HAVE_BOOLEAN seems like something we should keep? In case someone there is using whatever UNIXWARE is
> HAVE_PTHREAD is not used Ok
> Use -pthread instead of -lpthread when compiling Ok
> Make poppler compile of threads not available I don't understand this one, please explain the rationale of it
> Remove unused HAVE_ZLIB_H/HAVE_LIBZ Ok
> move macros out of poppler-config.h that don't need to be there ok
> Move strtok_r emulation to goo/glibc Ok
> Use cmath and remove fmin/fmax from poppler-config.h Ok
> stdio.h not needed in poppler-config.h Are you totally sure about that? Who defines the printf __format__ attributes? i've no idea but seems like they may actually come from stdio.h
> c++ has long long ok
> use <cmath> for isfinite Ok
> Remove VC7 workaround Ok
> mingw threads fix ok
> mingw warnings fix ok
Created attachment 134903 [details] [review] Fix warning: comparison of unsigned expression v2 Restore const.
Created attachment 134904 [details] [review] Fix warning implicit declaration of function v2 Add explanation to commit
(In reply to Albert Astals Cid from comment #30) > > Fix deprecated warnings in glib demo > > I'll leave this for someone that knows gstuff Carlos, could you review this. Also there is one glib demo deprecated warning remaining that I don't know how to fix.
(In reply to Albert Astals Cid from comment #31) > > Fix some -Wundef warnings > > I'm actually kind of scared of this patch, we kind of know "it works" as of > now, and changing this is kind of fragile, but ok i guess I don't want to commit anything your not entirely happy with. What specifically concerns you about the patch? It just changes #if ... to #if defined() or #ifdef for the feature macros. It is less fragile than the existing code where someone could make a typo eg #if ENABLED_LIBZ /* should be ENABLED_ZLIB */ and the code will compile without warnings.
(In reply to Albert Astals Cid from comment #32) > > Use _WIN32 instead of WIN32 > > I've no idea about windows, if you know this is correct go ahead, if not > sure find someone else to have a look :) It is correct. We did fix most of them a long time ago but a few got missed (or added later - I didn't check the history).
(In reply to Albert Astals Cid from comment #33) > > Remove unused t1lib code for the obsolete t1lib library > > -// or cmake: DISABLE_OUTLINE, DEBUG_MEM, SPLASH_CMYK, HAVE_T1LIB_H, > +// or cmake: DISABLE_OUTLINE, DEBUG_MEM, > > You removed SPLASH_CMYK by mistake i guess It wasn't a mistake. SPLASH_CMYK is now an option defined in CMakeLists.txt. > > otherwise, fine
(In reply to Albert Astals Cid from comment #38) > > Remove unused config.h macros > > the HAVE_BOOLEAN seems like something we should keep? In case someone there > is using whatever UNIXWARE is UnixWare removed "boolean" from user namespace in 2004: ftp://ftp.sco.com/pub/unixware7/713/uw713up/relnotes-up4.html "104. Change the "enum boolean" tag in /usr/include/sys/types.h to "enum __boolean", removing the type/tag "boolean" from the user name space." I don't think it likely that anyone would try building a 2017 poppler on a 2004 UnixWare, particularly with the C++11 requirement.
(In reply to Albert Astals Cid from comment #41) > > Make poppler compile of threads not available > > I don't understand this one, please explain the rationale of it We've got 77 "#ifdef MULTITHREADED" macros all over the code but MULTITHREADED is always defined. We should either define MULTITHREADED based on whether threads have been detected or just remove the macro. The former allows users to build on systems without threads. Although the best solution IMHO is too add a no op version of the mutex class to GooMutex.h for when threads are not available then all the MULTITHREADED macros can be removed.
Created attachment 134906 [details] [review] mingw warnings fix v2 (In reply to Albert Astals Cid from comment #46) > > stdio.h not needed in poppler-config.h > Are you totally sure about that? Who defines the printf __format__ > attributes? i've no idea but seems like they may actually come from stdio.h I found when compiling in mingw it is needed and added it back just above the MINGW_PRINTF_FORMAT with a comment explaining why it is needed. Removed the stdio patch and rebased the mingw warnings patch.
(In reply to Adrian Johnson from comment #55) > (In reply to Albert Astals Cid from comment #31) > > > Fix some -Wundef warnings > > > > I'm actually kind of scared of this patch, we kind of know "it works" as of > > now, and changing this is kind of fragile, but ok i guess > > I don't want to commit anything your not entirely happy with. What > specifically concerns you about the patch? It just changes #if ... to #if > defined() or #ifdef for the feature macros. It is less fragile than the > existing code where someone could make a typo eg > #if ENABLED_LIBZ /* should be ENABLED_ZLIB */ > > and the code will compile without warnings. Before a #define Foo 0 would go in one way of the branch and now would go on the other, but since the build system doens't really create any define to 0 i guess we're fine
(In reply to Adrian Johnson from comment #59) > (In reply to Albert Astals Cid from comment #41) > > > Make poppler compile of threads not available > > > > I don't understand this one, please explain the rationale of it > > We've got 77 "#ifdef MULTITHREADED" macros all over the code but > MULTITHREADED is always defined. We should either define MULTITHREADED based > on whether threads have been detected or just remove the macro. The former > allows users to build on systems without threads. > > Although the best solution IMHO is too add a no op version of the mutex > class to GooMutex.h for when threads are not available then all the > MULTITHREADED macros can be removed. Hmmm, ok i guess, to be honest i think we shouldn't really worry much about the non multithreaded case, how did you test this? which system without thread support did you use?
Ok, so everything except the gtk demo look ok to me and that one i don't have any idea but if it fixes it for you i guess you can commit, if it breaks, well it's the demo after all and we can fix it.
Pushed
Comment on attachment 134581 [details] [review] Fix deprecated warnings in glib demo Review of attachment 134581 [details] [review]: ----------------------------------------------------------------- ::: glib/demo/utils.c @@ +274,5 @@ > uri = g_file_get_uri (file); > g_object_unref (file); > if (uri) { > +#if GTK_CHECK_VERSION(3, 22, 0) > + gtk_show_uri_on_window (GTK_WINDOW(gtk_widget_get_window (button)), This is not correct. gtk_show_uri_on_window() expects a GtkWindow, but gtk_widget_get_window() returns a GdkWindow (which is not a toplevel window). To get the toplevel window of a widget you can use something like this: toplevel = gtk_widget_get_toplevel (button); gtk_show_uri_on_window (gtk_widget_is_toplevel (toplevel) ? GTK_WINDOW (toplevel) : NULL, @@ +542,5 @@ > uri = g_file_get_uri (file); > g_object_unref (file); > if (uri) { > +#if GTK_CHECK_VERSION(3, 22, 0) > + gtk_show_uri_on_window (GTK_WINDOW(gtk_widget_get_window (button)), Ditto.
Created attachment 134985 [details] [review] glib demo: correct the previous warnings fix Does this look good? Also, do you have any idea how to fix this warning? ../glib/demo/selections.c: In function ‘pgd_selections_drawing_area_realize’: ../glib/demo/selections.c:340:9: warning: ‘gtk_style_context_get_background_color’ is deprecated: Use 'gtk_render_background' instead [-Wdeprecated-declarations]
(In reply to Adrian Johnson from comment #66) > Created attachment 134985 [details] [review] [review] > glib demo: correct the previous warnings fix > > Does this look good? Yes. > Also, do you have any idea how to fix this warning? > > ../glib/demo/selections.c: In function ‘pgd_selections_drawing_area_realize’: > ../glib/demo/selections.c:340:9: warning: > ‘gtk_style_context_get_background_color’ is deprecated: Use > 'gtk_render_background' instead [-Wdeprecated-declarations] You can probably do: GdkRGBA *rgba; gtk_style_context_get (context, GTK_STATE_FLAG_SELECTED, "background-color", &rgba, NULL); ..... gdk_rgba_free (rgba);
Created attachment 135378 [details] [review] Fix selections warning Patch to fix the last remaining glib warning
(In reply to Adrian Johnson from comment #68) > Created attachment 135378 [details] [review] [review] > Fix selections warning > > Patch to fix the last remaining glib warning Thanks, please push it too.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.