Re: [macports-ports] branch master updated: qt4-mac: Fix pointer comparison with 0.

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [macports-ports] branch master updated: qt4-mac: Fix pointer comparison with 0.

Ryan Schmidt-24

> On May 24, 2017, at 09:28, Michael Dickens <[hidden email]> wrote:
>
> Michael Dickens (michaelld) pushed a commit to branch master
> in repository macports-ports.
>
>
> https://github.com/macports/macports-ports/commit/a12f8d370932d171fef29d833717b07b1b1e742a
>
> The following commit(s) were added to refs/heads/master by this push:
>
>      new a12f8d3  qt4-mac: Fix pointer comparison with 0.
>
> a12f8d3 is described below
>
>
> commit a12f8d370932d171fef29d833717b07b1b1e742a
>
> Author: Michael Dickens <[hidden email]>
> AuthorDate: Wed May 24 10:28:58 2017 -0400
>
>
>     qt4-mac: Fix pointer comparison with 0.
>
>    
>
>     Addresses ticket https://trac.macports.org/ticket/54183 .
>
> ---
>  aqua/qt4-mac/Portfile                              |  5 +++++
>  .../files/patch-fix_pointer_comparison_to_0.diff   | 22 ++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
>
> diff --git a/aqua/qt4-mac/Portfile b/aqua/qt4-mac/Portfile
>
> index afd319d..da02bc8 100644
>
> --- a/aqua/qt4-mac/Portfile
>
> +++ b/aqua/qt4-mac/Portfile
>
> @@ -287,6 +287,11 @@ platform darwin {
>
>  
>  patchfiles-append patch-better-invalid-fonttable-handling.diff
>  
>
> +# (29) Fix pointer comparison with 0
>
> +# These error out on clang 4.0, whereas they didn't in 3.9.
>
> +
>
> +patchfiles-append patch-fix_pointer_comparison_to_0.diff
>
> +
>
>  # error out if trying to build on a new OSX version (> 10.12).
>  
>  platform darwin {
>
> diff --git a/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff b/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff
>
> new file mode 100644
>
> index 0000000..4ed86aa
>
> --- /dev/null
>
> +++ b/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff
>
> @@ -0,0 +1,22 @@
>
> +--- src/3rdparty/webkit/Source/WebCore/html/HTMLImageElement.cpp.orig
>
> ++++ src/3rdparty/webkit/Source/WebCore/html/HTMLImageElement.cpp
>
> +@@ -74,7 +74,7 @@
>
> +     RefPtr<HTMLImageElement> image = adoptRef(new HTMLImageElement(imgTag, document));
>
> +     if (optionalWidth)
>
> +         image->setWidth(*optionalWidth);
>
> +-    if (optionalHeight > 0)
>
> ++    if (optionalHeight)
>
> +         image->setHeight(*optionalHeight);
>
> +     return image.release();
>
> + }

I agree that this change looks equivalent, but:


> +--- tools/linguist/linguist/messagemodel.cpp.orig
>
> ++++ tools/linguist/linguist/messagemodel.cpp
>
> +@@ -183,7 +183,7 @@
>
> +         if (ContextItem *c = one->findContext(oc->context())) {
>
> +             for (int j = 0; j < oc->messageCount(); ++j) {
>
> +                 MessageItem *m = oc->messageItem(j);
>
> +-                if (c->findMessage(m->text(), m->comment()) >= 0)
>
> ++                if (c->findMessage(m->text(), m->comment()))
>
> +                     ++inBoth;
>
> +             }
>
> +         }
>

This change does not. Checking for >= 0 is not the same as checking for Boolean true. So this changes the behavior of the compiled code, doesn't it? So the port's revision should increase?


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [macports-ports] branch master updated: qt4-mac: Fix pointer comparison with 0.

Mihai Moldovan-2
On 05/27/2017 11:08 AM, Ryan Schmidt wrote:

>> diff --git a/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff b/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff
>>
>> new file mode 100644
>>
>> index 0000000..4ed86aa
>>
>> --- /dev/null
>>
>> +++ b/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff
>>
>> @@ -0,0 +1,22 @@
>>
>> +--- src/3rdparty/webkit/Source/WebCore/html/HTMLImageElement.cpp.orig
>>
>> ++++ src/3rdparty/webkit/Source/WebCore/html/HTMLImageElement.cpp
>>
>> +@@ -74,7 +74,7 @@
>>
>> +     RefPtr<HTMLImageElement> image = adoptRef(new HTMLImageElement(imgTag, document));
>>
>> +     if (optionalWidth)
>>
>> +         image->setWidth(*optionalWidth);
>>
>> +-    if (optionalHeight > 0)
>>
>> ++    if (optionalHeight)
>>
>> +         image->setHeight(*optionalHeight);
>>
>> +     return image.release();
>>
>> + }
>
> I agree that this change looks equivalent, but:
Strictly speaking, even > 0 is not equivalent to ((optionalHeight) =>
(optionalHeight != 0)) for this signed type.



Mihai


signature.asc (945 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [macports-ports] branch master updated: qt4-mac: Fix pointer comparison with 0.

Michael Dickens-4
What both of you write is probably technically correct. These are
patches from elsewhere and, in my reading, make sense based on context.

Rev-bump? Maybe; not clear if that's really necessary. We can always do
that, but since it's Qt4 I'm inclined to not do so unless someone
complains. These fixes were, after all, really just to get qt4-mac
compiling using macports-clang-4.0.

That said, I'll rev-bump if folks really think it's necessary. I just
hate doing so any more often than necessary for qt4-mac since it takes
so long to build. - MLD
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [macports-ports] branch master updated: qt4-mac: Fix pointer comparison with 0.

Rainer Müller-4
In reply to this post by Mihai Moldovan-2
On 05/28/2017 05:34 PM, Mihai Moldovan wrote:

> On 05/27/2017 11:08 AM, Ryan Schmidt wrote:
>>> diff --git a/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff b/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff
>>>
>>> new file mode 100644
>>>
>>> index 0000000..4ed86aa
>>>
>>> --- /dev/null
>>>
>>> +++ b/aqua/qt4-mac/files/patch-fix_pointer_comparison_to_0.diff
>>>
>>> @@ -0,0 +1,22 @@
>>>
>>> +--- src/3rdparty/webkit/Source/WebCore/html/HTMLImageElement.cpp.orig
>>>
>>> ++++ src/3rdparty/webkit/Source/WebCore/html/HTMLImageElement.cpp
>>>
>>> +@@ -74,7 +74,7 @@
>>>
>>> +     RefPtr<HTMLImageElement> image = adoptRef(new HTMLImageElement(imgTag, document));
>>>
>>> +     if (optionalWidth)
>>>
>>> +         image->setWidth(*optionalWidth);
>>>
>>> +-    if (optionalHeight > 0)
>>>
>>> ++    if (optionalHeight)
>>>
>>> +         image->setHeight(*optionalHeight);
>>>
>>> +     return image.release();
>>>
>>> + }
>>
>> I agree that this change looks equivalent, but:
>
> Strictly speaking, even > 0 is not equivalent to ((optionalHeight) =>
> (optionalHeight != 0)) for this signed type.

It is actually a const int * optionalHeight. Comparing a pointer is to an int is
bogus and therefore the compiler complains about this expression. The fix in
this hunk seems fine to me.

This hunk seems to originate at WebKit upstream and merely trickled down to Qt:
https://trac.webkit.org/changeset/113848/webkit

Rainer
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [macports-ports] branch master updated: qt4-mac: Fix pointer comparison with 0.

Rainer Müller-4
In reply to this post by Michael Dickens-4
On 05/28/2017 05:50 PM, Michael Dickens wrote:
> What both of you write is probably technically correct. These are
> patches from elsewhere and, in my reading, make sense based on context.

So this is a direct backport from upstream as far as I see [1]. Note that the
method in question ContextItem::findMessage() returns a MessageItem* [2], so
this was previously comparing a pointer with an int.

> Rev-bump? Maybe; not clear if that's really necessary. We can always do
> that, but since it's Qt4 I'm inclined to not do so unless someone
> complains. These fixes were, after all, really just to get qt4-mac
> compiling using macports-clang-4.0.

I have no idea what other compilers produce for ptr >= 0, though. While this
helps to gets it to compile with macports-clang-4.0, it may also change behavior
for others. A rev-bump would be the safest option.

As a side note, I am kind of surprised that it took 5 years for this fix to
reach us. I guess Qt 4.x is entirely unmaintained by now and will not get
bugfixes from upstream? This is especially worrying me as that means the
embedded WebKit does not receive security updates...

Rainer

[1]
https://code.qt.io/cgit/qt/qttools.git/commit/?id=7138c963f9d1258bc1b49cb4d63c3e2b7d0ccfda
[2]
https://code.qt.io/cgit/qt/qttools.git/tree/src/linguist/linguist/messagemodel.h?id=7138c963f9d1258bc1b49cb4d63c3e2b7d0ccfda#n125
Loading...