[poppler] Openjpeg2 patches: Why de-inline doGetChar?

[Albert Astals Cid]

El Dilluns, 5 de gener de 2015, a les 23:34:07, Adam Reichold va escriure:
> Hello,
> 
> Am 05.01.2015 um 23:02 schrieb Adam Reichold:
> > Hello,
> > 
> > Am 05.01.2015 um 22:49 schrieb Albert Astals Cid:
> >> El Dilluns, 5 de gener de 2015, a les 22:29:22, Adam Reichold va
> >> 
> >> escriure:
> >>> Hello again,
> >>> 
> >>> Am 05.01.2015 um 21:59 schrieb Albert Astals Cid:
> >>>> El Dilluns, 5 de gener de 2015, a les 21:46:32, Adam
> >>>> Reichold va
> >>>> 
> >>>> escriure:
> >>>>> Hello again,
> >>>>> 
> >>>>> suggested changes attached as patch.
> >>>> 
> >>>> Doesn't compile, init() is not accessible from that static
> >>>> function.
> >>> 
> >>> Sorry about that, it seems CMake does not find OpenJPEG (1 or
> >>> 2) at all on my system with the current master, the library is
> >>> found alright but the includes are
> >>> "LIBOPENJPEG_INCLUDE_DIR-NOTFOUND" so the file was not compiled
> >>> at all. Autoconf on the other hand not problems finding at
> >>> least the preferred OpenJPEG version 1.
> >> 
> >> HAve you tried cleaning your cmake build dir? It works fine to
> >> me.
> > 
> > It seems for some reason Arch Linux installs the OpenJPEG header
> > only into "/usr/include/openjpeg-1.5/openjpeg.h" and "find_path"
> > does not find that, symlinking "/usr/include/openjpeg.h" there
> > works fine. (Possibly nobody noticed as the package is built using
> > Autoconf?) (Also I have to correct myself, OpenJPEG version 2 is
> > found correctly.)
> 
> It seems the difference is that "configure.ac" will try pkg-config for
> OpenJPEG version 1 first and then fall back to heuristics whereas
> "FindLIBOPENJPEG.cmake" does not try pkg-config at all?

openjpeg didn't always have a pkg-config file i think, that may be the reason.

Cheers,
  Albert

> 
> Best regards, Adam.
> 
> > Best regards, Adam.
> > 
> >> Cheers, Albert
> >> 
> >>> I also attached a fixed patch that moves the
> >>> lazy-initialization into the entry points which also has the
> >>> benefit of making the check only once for each call of
> >>> 'getChars' but is admittedly uglier. (One solution would be to
> >>> move the 'inited' flag into the class proper instead of the
> >>> pimpl or even use "priv != NULL" as the initialization
> >>> condition, but that felt unnecessarily intrusive.)
> >>> 
> >>> Best regards, Adam.
> >>> 
> >>>> Cheers, Albert
> >>>> 
> >>>>> Best regards, Adam.
> >>>>> 
> >>>>> Am 05.01.2015 um 21:24 schrieb Adam Reichold:
> >>>>>> Hello,
> >>>>>> 
> >>>>>> Am 04.01.2015 um 23:44 schrieb Albert Astals Cid:
> >>>>>>> El Dilluns, 5 de gener de 2015, a les 08:53:26, Adrian
> >>>>>>> 
> >>>>>>>  Johnson
> >>>>>>> 
> >>>>>>> va escriure:
> >>>>>>>> On 05/01/15 08:39, Albert Astals Cid wrote:
> >>>>>>>>> Adrian, any reason you de-inlined doGetChar in the
> >>>>>>>>> 
> >>>>>>>>>  patches for openjpeg2?
> >>>>>>>>> 
> >>>>>>>>> As far as i remember when i made this function
> >>>>>>>>> inline it got us a not so bad free speed increase.
> >>>>>>>> 
> >>>>>>>> When I wrote the patch I moved all openjpeg specific
> >>>>>>>> types and code into the .cc file. I could not figure
> >>>>>>>> out how to make an inline function in the .h access
> >>>>>>>> the JPXStreamPrivate struct that is defined in the
> >>>>>>>> .cc file.
> >>>>>>> 
> >>>>>>> I see :/
> >>>>>>> 
> >>>>>>> Cheers, Albert
> >>>>>> 
> >>>>>> Looking at the implemenation, both "doGetChar" and
> >>>>>> "doLookChar" will always be called inside
> >>>>>> "JPEG2000Stream.cc" via the virtual methods "getChar(s)"
> >>>>>> and "lookChar", so I'd say we could just mark the
> >>>>>> implemenations as "inline" as the compiler can still
> >>>>>> inline within the compilation unit.
> >>>>>> 
> >>>>>> Actually, I think there is no compelling reason to have
> >>>>>> those present in the header at all, as they always seem
> >>>>>> to access the data via the "priv" pointer anyway, so I'd
> >>>>>> say the declarations like
> >>>>>> 
> >>>>>> static inline int doGetChar(JPXStreamPrivate* priv);
> >>>>>> static inline int doLookChar(JPXStreamPrivate* priv);
> >>>>>> 
> >>>>>> only within the source file could open even more
> >>>>>> possibilities for compiler optimizations. (You could
> >>>>>> probably also make them inline methods of
> >>>>>> "JPXStreamPrivate" as well, but then the compiler would
> >>>>>> have to infer the effective linkage. Maybe we could put
> >>>>>> the whole definition of "JPXStreamPrivate" within the
> >>>>>> anonymous namespace anyway?)
> >>>>>> 
> >>>>>> Best regards, Adam.
> >>>>>> 
> >>>>>>>>> Cheers,
> >>>>>>>>> 
> >>>>>>>>> Albert
> >>>>>>>>> 
> >>>>>>>>> _______________________________________________
> >>>>>>>>> poppler mailing list poppler at lists.freedesktop.org
> >>>>>>>>> 
> >>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________ poppler
> 
> >>>>>>>> mailing list poppler at lists.freedesktop.org
> >>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________ poppler
> 
> >>>>>>> mailing list poppler at lists.freedesktop.org
> >>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>> 
> >>>>>> _______________________________________________ poppler
> >>>>>> mailing list poppler at lists.freedesktop.org
> >>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> >> 
> >> _______________________________________________ poppler mailing
> >> list poppler at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/poppler
> > 
> > _______________________________________________ poppler mailing
> > list poppler at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler