[Ocs] Review Request: Added static build option

[Laszlo Papp]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101880/#review4535
-----------------------------------------------------------



CMakeLists.txt
<http://git.reviewboard.kde.org/r/101880/#comment3876>

    I would use "Build a static library" in order to be consistent to the ATTICA_STATIC_BUILD name just like ATTICA_ENABLE_TESTS is consistent below.



lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101880/#comment3877>

    1) I would leave out "client" word both times. Attica is a client only, so it is implied in my opinion. Also it is used as ATTICA_ENABLE_TESTS without client, also in your addition: ATTICA_STATIC_BUILD. Could you please rename the file accordingly ?
    
    2) Is it intended to use the source dir instead of the binary dir ? See one line above ;-)



lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101880/#comment3878>

    I think ATTICA_STATIC_BUILD is talkative enough so that it is no need to be commented. Actually one of the most straight-forward snippet here :)



lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101880/#comment3879>

    1) Same as above, we can simplify it by leaving out that comment line.
    
    add_library(... STATIC/SHARED ...) is also recognizable on the top of the talkative variable name in the if statement
    
    2) Small tricks, like indentation compared to the if/else lines, make the code more readable.



lib/atticaclient_export.h.cmake
<http://git.reviewboard.kde.org/r/101880/#comment3880>

    It seems you put some unneccesary empty lines here. :)



lib/atticaclient_export.h.cmake
<http://git.reviewboard.kde.org/r/101880/#comment3881>

    I have just had a look at the ./kdecore/kdecore_export.h file and that is how it looks like:
    
    #ifndef KDECORE_EXPORT
    # if defined(KDELIBS_STATIC_LIBS)
       /* No export/import for static libraries */
    #  define KDECORE_EXPORT
    # elif defined(MAKE_KDECORE_LIB)
       /* We are building this library */ 
    #  define KDECORE_EXPORT KDE_EXPORT
    # else
       /* We are using this library */ 
    #  define KDECORE_EXPORT KDE_IMPORT
    # endif
    #endif
    
    According to this, I would write this:
    
    #ifndef ATTICA_EXPORT
    # if ${ATTICA_STATIC_BUILD}
       /* No export/import for static libraries */
    #  define ATTICA_EXPORT
    # elif defined(ATTICA_LIB_MAKEDLL)
       /* We are building this library */ 
    #  define ATTICA_EXPORT Q_DECL_EXPORT
    # else
       /* We are using this library */ 
    #  define ATTICA_EXPORT Q_DECL_IMPORT
    # endif
    #endif
    
    It has more advantages, for instance:
    1) It looks much simplier
    2) Personally it is nice so that it is consistent with that what is done in the kdelibs, at least for me =p.
    
    First I thought you meant to make it a regular header, but if you run it through the configure_file, it should be fine.
    
    Good patch overall and sorry for nitpicks ;-)


- Laszlo


On July 8, 2011, 6:51 p.m., Constantin-Alexandru Tudorica wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101880/
> -----------------------------------------------------------
> 
> (Updated July 8, 2011, 6:51 p.m.)
> 
> 
> Review request for Attica and Patrick Spendrin.
> 
> 
> Summary
> -------
> 
> Added static build options for libattica.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt d45455a 
>   lib/CMakeLists.txt a04167f 
>   lib/atticaclient_export.h bb2e6f1 
>   lib/atticaclient_export.h.cmake PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101880/diff
> 
> 
> Testing
> -------
> 
> Did a successful build on windows using mingw32 and also linked successfully to the static library. 
> 
> 
> Thanks,
> 
> Constantin-Alexandru
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/ocs/attachments/20110708/029f0829/attachment-0001.htm>