Review request

[Raphael Kubo da Costa]

At Mon, 6 Dec 2010 14:51:12 +0100,
Thomas Baumgart wrote:
> Hi folks,
> 
> I have just moved libalkimia as part of Alkimia (see 
> http://techbase.kde.org/Projects/KdeFinance/Alkimia for details)  to kdereview 
> for further processing by the community.

alkvalue.h
==========

  #define ALKIMIA_EXPORT KDE_EXPORT

    Isn't it preferable to create an alk_export.h, as is more usual
    for libraries?

  RoundTrunc,                  /**<
    Isn't it better to expand it to RoundTruncate?

  AlkValue convertDenom(const int denom = 100, const RoundingMethod how = RoundRound) const;
    convertDenominator?

  AlkValue convertPrec(const int precision = 2, const RoundingMethod how = RoundRound) const;
    convertPrecision?

  /// convert a denomination to a precision
    denomination or denominator?

  static mpz_class denomToPrec(mpz_class denom);
    denominatorToPrecision?

  static mpz_class precToDenom(mpz_class prec);
    precisionToDenominator?

alkvalue.cpp
============

  // qDebug("we got '%s' to convert", qPrintable(str));
    Don't forget to replace those with kDebug() when needed :)

CMakeLists.txt
==============

  * In general (at least if you follow the CMake conventions for
    kdelibs), the CMake commands should be lowercased.

  * Isn't it missing a project(...) call?

  FIND_PACKAGE(PkgConfig)
  FIND_PACKAGE(GMP)

    I'd rather include(MacroLibrary) and then use
    macro_optional_find_package() and macro_log_feature().

  include(${QT_USE_FILE})

    Hmm. I'd use the following:

        add_definitions(${QT_DEFINITIONS} ${KDE4_DEFINITIONS})
        include_directories(${KDE4_INCLUDES})

    instead. The include_directories() was really missing (it did not
    compile here without it).

  ADD_LIBRARY(alkimia SHARED ${alkimia_LIB_SRCS})

    Prefer kde4_add_library().

  IF(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
    SET(CMAKE_INSTALL_PREFIX "${KDE4_INSTALL_DIR}" CACHE PATH
      "Alkimia install prefix defaults to the KDE4 install prefix: ${KDE4_INSTALL_DIR}" FORCE)
  ENDIF(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)

    Why do you need this?

  # the RPATH to be used when installing
  # (cf. http://www.vtk.org/Wiki/CMake_RPATH_handling)
  SET(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib")
  # add the automatically determined parts of the RPATH
  # which point to directories outside the build tree to the install RPATH
  SET(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)

    Is this really needed? I remember a lot of RPATH-related
    discussions in the kde-buildsystem mailing list, but never really
    watched them. It might be worth investigating if this is
    necessary.

  # If no build type is set, use "Release with Debug Info"
  IF(NOT CMAKE_BUILD_TYPE)
    SET(CMAKE_BUILD_TYPE RelWithDebInfo)
  ENDIF(NOT CMAKE_BUILD_TYPE)
  [...]

    All the CMAKE_BUILD_TYPE-related code (and the
    CMAKE_SHARED_LINKER_FLAGS stuff etc) should be handled
    automatically by FindKDE4Internal.cmake.

  IF( KDE4_BUILD_TESTS )

    Already handled by KDE4Macros.cmake

  # INCLUDE(CTest) # (makes sense only with a ctest online dashboard)
  ENABLE_TESTING()

    Already done by KDE4Defaults.cmake -- remember to
    include(KDE4Defaults) at the beginning.

  ADD_DEPENDENCIES( alkvaluetest alkimia )

    Shouldn't it be handled automatically by the target_link_libraries() call?

I recommend taking a look at the top-level CMakeLists.txt's for SC
modules (such as kdeutils) to see what kind of "bootstrapping" is
usually done. And, of course, if you have any questions, please ask
the kde-buildsystem mailing list.