Bug 60820 – Handle WTF_FEATURE defined to nothing

• Component: New Bugs
• Status: RESOLVED FIXED

[Lucas De Marchi]

Handle WTF_FEATURE defined to nothing

[Lucas De Marchi]

Created attachment 93535 [details]
Patch

[Lucas De Marchi]

Simple source code to test the result. BLA and BLO are considered to be disabled, so the output will be "bli\n"

---
#include <stdio.h>

#define ENABLE_BLA 0
#define ENABLE_BLI 1
#define ENABLE_BLO

#define ENABLE(WTF_FEATURE) (defined ENABLE_##WTF_FEATURE && ENABLE_##WTF_FEATURE +0)


int main(void)
{
#if ENABLE(BLA)
    printf("bla\n");
#endif
#if ENABLE(BLI)
    printf("bli\n");
#endif
#if ENABLE(BLO)
    printf("blo\n");
#endif

    return 0;
}

[Patrick R. Gansterer]

Comment on attachment 93535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93535&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        * wtf/Platform.h: ditto.

ditto what??

> Source/JavaScriptCore/wtf/Platform.h:55
> +#define ENABLE(WTF_FEATURE) (defined ENABLE_##WTF_FEATURE  && ENABLE_##WTF_FEATURE +0)

I don't like this patch. IMHO its a failure if a ENABLE_XXX is not defined. The buildsystem must take care of the defines.

[Lucas De Marchi]

(In reply to comment #3)
> (From update of attachment 93535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93535&action=review
> 
> > Source/JavaScriptCore/ChangeLog:17
> > +        * wtf/Platform.h: ditto.
> 
> ditto what??

I explained what I did above of this. This is the only file modified, so it directly applies to what I said before.


> 
> > Source/JavaScriptCore/wtf/Platform.h:55
> > +#define ENABLE(WTF_FEATURE) (defined ENABLE_##WTF_FEATURE  && ENABLE_##WTF_FEATURE +0)
> 
> I don't like this patch. IMHO its a failure if a ENABLE_XXX is not defined. The buildsystem must take care of the defines.

I think you misread the issue. Please see the discussion on mailing list about adding other ports to CMake like GTK and Android.

ENABLE(WTF_FEATURE) checks if the the feature is defined *and* TRUE. This check is currently failing in case the feature is defined but without any value. What this patch does is to fix this.

It looks very hackish, but I don't think there's a fix more elegant than this.

[Steve Block]

Comment on attachment 93535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93535&action=review

>>> Source/JavaScriptCore/wtf/Platform.h:55
>>> +#define ENABLE(WTF_FEATURE) (defined ENABLE_##WTF_FEATURE  && ENABLE_##WTF_FEATURE +0)
>> 
>> I don't like this patch. IMHO its a failure if a ENABLE_XXX is not defined. The buildsystem must take care of the defines.
> 
> I think you misread the issue. Please see the discussion on mailing list about adding other ports to CMake like GTK and Android.
> 
> ENABLE(WTF_FEATURE) checks if the the feature is defined *and* TRUE. This check is currently failing in case the feature is defined but without any value. What this patch does is to fix this.
> 
> It looks very hackish, but I don't think there's a fix more elegant than this.

I didn't follow all of the details of your webkit-dev discussion about cmake, but this patch looks wrong to me. The code is written expecting these flags to be either undefined, or set to TRUE/FALSE. Surely this should be solved by fixing cmake to set the flags correctly, not modifying the code to compensate.

[Lucas De Marchi]

(In reply to comment #5)
> I didn't follow all of the details of your webkit-dev discussion about cmake, but this patch looks wrong to me. The code is written expecting these flags to be either undefined, or set to TRUE/FALSE. Surely this should be solved by fixing cmake to set the flags correctly, not modifying the code to compensate.

Ok. So the only way to have this working right in CMake for all ports is to define a default value (0 ?) for every feature. It's not my preferred solution (it seems much cleaner to do what I did here) but it sure can be made.

[Lucas De Marchi]

Created attachment 96008 [details]
Patch

[Leandro Pereira]

(In reply to comment #7)
> Created an attachment (id=96008) [details]
> Patch

LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 96008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96008&action=review

> ChangeLog:18
> +        with another variable. This way the feature will be always 0 or 1 and

will always be

[Lucas De Marchi]

Committed r88342: <http://trac.webkit.org/changeset/88342>