KDE/kdevplatform

[Andreas Pakulat]

On 03.08.09 22:57:29, Aleix Pol Gonzalez wrote:
> SVN commit 1006580 by apol:
> 
> Properly look for Kompare, the CMake way.

Why? IMHO its just unnecessary overhead compared to what "finding
kompare" actually means. All we need is find 1 single header file. 

Apart from that:
 
> +macro_log_feature(KOMPARE_FOUND "Kompare" "KPart to view file differences"
> +                        "http://www.caffeinated.me.uk/kompare/" FALSE ""
> +                        "Required for difference checking")

This website is completely outdated, if we put a webpage here it should
at least point to the right sources and list the current developers. 

> +configure_file( ${CMAKE_CURRENT_SOURCE_DIR}/config-kdevplatform.h.cmake
> +                ${CMAKE_CURRENT_BINARY_DIR}/config-kdevplatform.h )
> +

Even more code to execute.

>  include (KDE4Defaults)
>  include (MacroWriteBasicCMakeVersionFile)
>  include (MacroLibrary)

Why is the above stuff before these? It should be after it.

> -macro_log_feature(KOMPARE_INCLUDES "Kompare Part" "Enhance code generation user feedback" "" FALSE)
> -if(ENABLE_KOMPARE)
> -    message(STATUS "Kompare interfaces FOUND. Enabling Kompare Support ")
> -    add_definitions(-DKOMPARE_ENABLED=true)
> -else(ENABLE_KOMPARE)
> -    message(STATUS "Kompare interfaces NOT FOUND. Kompare Support cannot be enabled")
> -    add_definitions(-DKOMPARE_ENABLED=false)
> -endif(ENABLE_KOMPARE)
> -
> -
>  set( kdevplatformlanguage_LIB_UI
>      codegen/ui/newclass.ui
>      codegen/ui/overridevirtuals.ui
> --- trunk/KDE/kdevplatform/language/codegen/komparesupport.cpp #1006579:1006580
> @@ -18,8 +18,9 @@
>   */
>  
>  #include "komparesupport.h"
> +#include <config-kdevplatform.h>
>  
> -#if KOMPARE_ENABLED == true
> +#ifdef HAVE_KOMPARE
>  
>  #include <stddef.h>
>  #include <cstddef>
> --- trunk/KDE/kdevplatform/language/codegen/komparesupport.h #1006579:1006580
> @@ -20,6 +20,8 @@
>  #ifndef KOMPARESUPPORT_H
>  #define KOMPARESUPPORT_H
>  
> +#include <config-kdevplatform.h>
> +
>  class QWidget;
>  class QString;
>  
> @@ -31,8 +33,12 @@
>  class KompareWidgets
>  {
>    public:
> -    static const bool enabled = KOMPARE_ENABLED;
> -    
> +#ifdef WITH_KOMPARE
> +    static const bool enabled = 1;
> +#else
> +    static const bool enabled = 0;
> +#endif
> +
>      KompareWidgets();
>      ~KompareWidgets();

This is _exactly_ what I do not want. the amount of ifdefs inside our
code should be kept to the absolute possible mininmum. Not to mention
that bool's actually should have proper true/fals initialization -
without relying on the compiler converting int to bool.

IMHO this should be reverted.

Andreas

-- 
Of course you have a purpose -- to find a purpose.