[KDE/Mac] Review Request 124894: [OS X] make it build

Fri Aug 28 05:35:17 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124894/#review84503
-----------------------------------------------------------

OK, it's getting closer to be ready now. Good job!

Couple of nitpicks which couldn't really fit in another comment:
 * I would rename memfn.h to mem_fn.h to be more aligned with the boost naming
 * Don't use preprocessor indentation like "#   ifdef" we don't do that in other places, same thing for the code in the preprocessor blocks.

src/utils/memfn.h (line 31)
<https://git.reviewboard.kde.org/r/124894/#comment58489>

    I'm still not in love with that, so I would say: either you manage to turn your get_pointer overload into a template where QSharedPointer doesn't appear, or you include QSharedPointer here.

    I know your point about "it shall not bring extra dependency" which is true but at the same time boost/mem_fn.hpp already brings deps on the std smart pointers it's not really different.

src/utils/memfn.h (line 51)
<https://git.reviewboard.kde.org/r/124894/#comment58485>

    Trailing whitespace

    Good call on the decltype BTW... dunno why I didn't think of it earlier. :-)

src/utils/memfn.h (lines 55 - 62)
<https://git.reviewboard.kde.org/r/124894/#comment58488>

    I would say this block is not needed, it's just making the read more complex. Let's settle only on the C++11 variant.

src/utils/memfn.h (line 59)
<https://git.reviewboard.kde.org/r/124894/#comment58486>

    Trailing whitespace

src/utils/memfn.h (line 66)
<https://git.reviewboard.kde.org/r/124894/#comment58487>

    Trailing whitespace

- Kevin Ottens

On Aug. 27, 2015, 9:07 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124894/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2015, 9:07 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and Zanshin.
> 
> 
> Repository: zanshin
> 
> 
> Description
> -------
> 
> This is a set of patches that allow zanshin version `0.2.1-1163-gbf5c321` to build on OS X against the git head (or almost head) of kdelibs 4.14 and kdepim* 4.14 .
> I presume for now that a number of changes are self-explanatory (for instance, `zanshinkdepimstatic` calls itself static, isn't installed and doesn't export any symbols explicitly so all points to it being intended to be a static instead of a shared lib).
> The patches that disable a number of tests, and I do not understand myself what exactly goes wrong without them other than that the linker gives errors like
> 
> ```
> # Undefined symbols for architecture x86_64:
> #   "decltype(*(std::__1::forward<QSharedPointer<Domain::QueryResult<QString> > >(fp0)).*fp(std::__1::forward<>(fp1))) std::__1::__invoke<QList<std::__1::function<void (QString, int)> > (Domain::QueryResult<QString>::*&)() const, QSharedPointer<Domain::QueryResult<QString> >, void>(QList<std::__1::function<void (QString, int)> > (Domain::QueryResult<QString>::*&&&)() const, QSharedPointer<Domain::QueryResult<QString> >&&)", referenced from:
> #       std::__1::__function::__func<std::__1::__mem_fn<QList<std::__1::function<void (QString, int)> > (Domain::QueryResult<QString>::*)() const>, std::__1::allocator<std::__1::__mem_fn<QList<std::__1::function<void (QString, int)> > (Domain::QueryResult<QString>::*)() const> >, QList<std::__1::function<void (QString, int)> > (QSharedPointer<Domain::QueryResult<QString> >)>::operator()(QSharedPointer<Domain::QueryResult<QString> >&&) in queryresulttest.o
> # ld: symbol(s) not found for architecture x86_64
> # clang: error: linker command failed with exit code 1 (use -v to see invocation)
> ```
> 
> Possibly a comparable to the reason why I had to define the `std::accumulate` functions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/kdepim/libkdepim/CMakeLists.txt cc8845a 
>   3rdparty/kdepim/libkdepim/tests/CMakeLists.txt 775ac0e 
>   CMakeLists.txt 71b16a8 
>   src/akonadi/akonadiserializer.cpp 5116fa5 
>   src/domain/queryresultprovider.h 5d0b02d 
>   src/presentation/errorhandler.cpp 5610040 
>   src/utils/memfn.h PRE-CREATION 
>   tests/testlib/akonadistoragetestbase.cpp 8573374 
>   tests/units/akonadi/akonadidatasourcequeriestest.cpp c2e87dc 
>   tests/units/widgets/applicationcomponentstest.cpp f972673 
> 
> Diff: https://git.reviewboard.kde.org/r/124894/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5 with the system compiler (`Apple LLVM version 6.0 (clang-600.0.57) (based on LLVM 3.5svn)`)
> 
> My initial reason to install this was to "check it out" and learn how to use it, so I have not done any further testing ...
> 
> 
> File Attachments
> ----------------
> 
> datasourcequeriestest.cpp preprocessed by Apple Clang 6
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/24/b2f3e229-ca5a-4e55-b33b-6504b2cdb5c7__datasourcequeriestest-AppleClang6.i
> datasourcequeriestest.cpp preprocessed by clang 3.6.1
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/24/807e30a4-aa34-45b4-9e5c-92020ee8c7f5__datasourcequeriestestClang3.6.i
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20150828/b46d8d8a/attachment-0001.html>