Bug 107365 – Simplify a list of negative PLATFORM() tests

• Component: Platform
• Status: RESOLVED FIXED

[Laszlo Gombos]

Document the list of ports that support Windows and use the documented list to simplify a serious of negative PLATFORM() tests into a simpler list of positive tests.

[Laszlo Gombos]

Created attachment 183609 [details]
proposed change

[Eric Seidel (no email)]

Comment on attachment 183609 [details]
proposed change

Why should we do this?

[Laszlo Gombos]

(In reply to comment #2)
> (From update of attachment 183609 [details])
> Why should we do this?

The change in Platform.h is basically an ASSERT for the build system. Self-documenting code.

The change in config.h IMHO makes the code more readable and maintainable. I would not feel comfortable making this change without the change in Platform.h.

[Eric Seidel (no email)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 183609 [details] [details])
> > Why should we do this?
> 
> The change in Platform.h is basically an ASSERT for the build system. Self-documenting code.
> 
> The change in config.h IMHO makes the code more readable and maintainable. I would not feel comfortable making this change without the change in Platform.h.

The config.h change is r=me.  The Platform.h change is what I'm confused by. :)  I'm just not sure why it's important to have a list of ports which support windows.  We don't have the same for any other platform. :)

[Laszlo Gombos]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 183609 [details] [details] [details])

> The Platform.h change is what I'm confused by. :)  I'm just not sure why it's important to have a list of ports which support windows.  

I do not really know the definite list of PLATFORM()s that support OS(WINDOWS) - I just made a best effort to list them. The only way to know is to run the test in Platform.h in all the bots/ports and see which one fails/who screams. I was thinking that perhaps we should leave this code in there so that the next guy making a similar change would not have this problem. If I made an error in my list of Windows ports than the change in config.h is wrong.

> We don't have the same for any other platform. :)

I can add a list for other OS()s. 

Another example from Platform.h:

#if OS(DARWIN) && !PLATFORM(GTK) && !PLATFORM(QT)
#define ENABLE_PURGEABLE_MEMORY 1
#endif

I do not really know which ports would have ENABLE_PURGEABLE_MEMORY turned on. If I could replace the test with:

#if OS(DARWIN) && (PLATFORM(MAC) || PLATFORM(WX))

than I would find the code more readable and maintainable. 


Not a big deal, I am OK either way, just wanted to make sure that I make my case.

[Eric Seidel (no email)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (From update of attachment 183609 [details] [details] [details] [details])

> #if OS(DARWIN) && !PLATFORM(GTK) && !PLATFORM(QT)
> #define ENABLE_PURGEABLE_MEMORY 1
> #endif
> 
> I do not really know which ports would have ENABLE_PURGEABLE_MEMORY turned on. If I could replace the test with:

I agree.  This is a big (and growing) problem for WebKit.  That's part of what:
https://bugs.webkit.org/show_bug.cgi?id=85456
was about.

I'm glad that others (yourself included) are looking into this, and trying to simplify our features system so that we can acxtually tell what's on/off for various ports!

[Laszlo Gombos]

Created attachment 183691 [details]
remove the change in Platform.h

[Eric Seidel (no email)]

Comment on attachment 183691 [details]
remove the change in Platform.h

LGTM.

[WebKit Review Bot]

Comment on attachment 183691 [details]
remove the change in Platform.h

Clearing flags on attachment: 183691

Committed r140283: <http://trac.webkit.org/changeset/140283>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Comment on attachment 183691 [details]
remove the change in Platform.h

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

> Source/WebCore/ChangeLog:11
> +        No new tests as there is no new functionality.

Please don't say that.
Some people literally believe they do not need tests for bug fix, or make up excuses. This particular sentence is becoming a red flag.

I think we can be better examples in our changelogs regarding testing/not testing.

[Eric Seidel (no email)]

(In reply to comment #11)
> (From update of attachment 183691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183691&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests as there is no new functionality.
> 
> Please don't say that.
> Some people literally believe they do not need tests for bug fix, or make up excuses. This particular sentence is becoming a red flag.
> 
> I think we can be better examples in our changelogs regarding testing/not testing.

Perhaps you should start a Webkit-dev thread on the subject. I was not aware of the concen.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 183691 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183691&action=review
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        No new tests as there is no new functionality.
> > 
> > Please don't say that.
> > Some people literally believe they do not need tests for bug fix, or make up excuses. This particular sentence is becoming a red flag.
> > 
> > I think we can be better examples in our changelogs regarding testing/not testing.
> 
> Perhaps you should start a Webkit-dev thread on the subject. I was not aware of the concen.

I failed to type my intended smilie. :)