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.
