KDE/kdevplatform

[Aleix Pol]

On Tue, Aug 4, 2009 at 1:23 AM, Andreas Pakulat <apaku at gmx.de> wrote:

> 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.

Yes, and in CMake we find using find_package.
I can change to find file if you prefer.


>
>
> 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.


Ok, I can change to: http://www.google.es/search?q=kompare
I agree we should specify it's on kdesdk. I'll add it later


>
> > +configure_file( ${CMAKE_CURRENT_SOURCE_DIR}/config-kdevplatform.h.cmake
> > +                ${CMAKE_CURRENT_BINARY_DIR}/config-kdevplatform.h )
> > +
>
> Even more code to execute.


I wonder what you mean. Every single cmake project use configure file for
conditional compilation inside files. I don't think is that much overhead
anyway. If cmake parsing (either cmake's or our's) is not fast enough is not
because of this (actually we don't do anything in configure_file).
I don't think it's superfluous anyway.


>
> >  include (KDE4Defaults)
> >  include (MacroWriteBasicCMakeVersionFile)
> >  include (MacroLibrary)
>
> Why is the above stuff before these? It should be after it.


I wanted to add it with the other find_packages. If you prefer to have it
below I'll move it. It is not a problem.


>
>
> > -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.


Fixed, my fault. Actually here I think that komparewidget shouldn't be
compiled at all, it was my next step.


>
> IMHO this should be reverted.
>

>
> Andreas
>
> --
> Of course you have a purpose -- to find a purpose.
>
> _______________________________________________
> KDevelop-devel mailing list
> KDevelop-devel at kdevelop.org
> https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20090804/ed0b3336/attachment.html>