CA broken; how do I differentiate NSValues for size and point?

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2
Hi,

Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:

2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)

CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.

Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)

I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.

Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.

- Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
- Is there a way out?
- Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)

Thanks

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Fred Kiefer
I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)

The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.

But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.

Hope this helps,
Fred

> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
>
> Hi,
>
> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
>
> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
>
> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
>
> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
>
> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
>
> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
>
> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
> - Is there a way out?
> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
>
> Thanks
> _______________________________________________
> Gnustep-dev mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Fred Kiefer
I added a bit of code in base that allows to use NSValue objects for size and point with methods for the other type. This is a bit closer to the Cocoa behaviour but will require more tweaking to have it fully correct. It will at least allow you to run this test code again.

> Am 20.05.2018 um 16:02 schrieb Fred Kiefer <[hidden email]>:
>
> I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)
>
> The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
> In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.
>
> But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.
>
> Hope this helps,
> Fred
>
>> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
>>
>> Hi,
>>
>> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
>>
>> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
>>
>> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
>>
>> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
>>
>> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
>>
>> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
>>
>> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
>> - Is there a way out?
>> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
>>
>> Thanks
>> _______________________________________________
>> Gnustep-dev mailing list
>> [hidden email]
>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>
>
> _______________________________________________
> Gnustep-dev mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2
I added a test to the -base from May 20:

  NSPoint point = {.x = 16.0, .y = 32.0};
  NSValue *pointV = [NSValue valueWithPoint: point];

  result = !strcmp(@encode(NSPoint), [pointV objCType]);
  PASS(result, "@encoding(NSPoint) == pointV objCType");

  result = strcmp(@encode(NSSize), [pointV objCType]);
  PASS(result, "@encoding(NSSize) == pointV objCType");

with this temporarly ine:
  NSLog(@"%s %s %s", @encode(NSPoint), @encode(NSSize), [pointV objCType]);

It passes and prints out:

2018-05-27 17:27:38.823 size-point-encoding[25395:25395] {_NSPoint=dd} {_NSSize=dd} {_NSPoint=dd}

I''m annoyed at how I did not notice that I did use CGSize. I will introduce support for NSSize/CGSize.

Thanks!

On Mon, May 21, 2018 at 12:53 AM Fred Kiefer <[hidden email]> wrote:
I added a bit of code in base that allows to use NSValue objects for size and point with methods for the other type. This is a bit closer to the Cocoa behaviour but will require more tweaking to have it fully correct. It will at least allow you to run this test code again.

> Am 20.05.2018 um 16:02 schrieb Fred Kiefer <[hidden email]>:
>
> I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)
>
> The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
> In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.
>
> But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.
>
> Hope this helps,
> Fred
>
>> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
>>
>> Hi,
>>
>> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
>>
>> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
>>
>> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
>>
>> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
>>
>> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
>>
>> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
>>
>> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
>> - Is there a way out?
>> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
>>
>> Thanks
>> _______________________________________________
>> Gnustep-dev mailing list
>> [hidden email]
>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>
>
> _______________________________________________
> Gnustep-dev mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2
So turns out we don't use shadowOffset in any demo code. So examining CAAnimation was pointless.

However I managed to get a backtrace using lldb, and this happens when the code sets the default shadowOffset value. So you pointed to the right place; shadowOffset is the problem. This is the place of the exception, in -[CALayer init] which calls +[CALayer defaultValueForKey:]:

  if ([key isEqualToString: @"shadowOffset"])                                                                
    {
      CGSize offset = CGSizeMake(0.0, -3.0);
      return [NSValue valueWithBytes: &offset objCType: @encode(CGSize)];
    }

What is strange is that this happens deep inside base, when I am setting valueWithBytes, with a CGSize type.

So I looked closer at the backtrace and it's happening in base/Source/Additions/GSObjCRuntime.m. Relevant chunk:

    frame #2: 0x00007ffff59e9d7d libgnustep-base.so.1.25`+[NSException raise:format:](self=0x00007ffff5f69878
, _cmd="S", name=0x00007ffff5f69438, format=0x00007ffff6003828) + 365 at NSException.m:1376
    frame #3: 0x00007ffff5b9ca2f libgnustep-base.so.1.25`-[NSObject(self=0x000000000148f568, _cmd="\xffffffa9
\x02", aSel="\x0e\x01") subclassResponsibility:] + 255 at NSObject+GNUstepBase.m:134
    frame #4: 0x00007ffff5b1d32b libgnustep-base.so.1.25`-[NSValue pointValue](self=0x000000000148f568, _cmd=
"\x0e\x01") + 43 at NSValue.m:394
    frame #5: 0x00007ffff5b58649 libgnustep-base.so.1.25`GSObjCSetVal(self=0x00000000014591c8, key="shadowOff
set", val=0x000000000148f568, sel="\xffffffda\e", type="{_NSSize=dd}", size=12, offset=0) + 3497 at GSObjCRun
time.m:1794
    frame #6: 0x00007ffff5a25857 libgnustep-base.so.1.25`SetValueForKey(self=0x00000000014591c8, anObject=0x0
00000000148f568, key="shadowOffset", size=12) + 1079 at NSKeyValueCoding.m:150
    frame #7: 0x00007ffff5a253ee libgnustep-base.so.1.25`-[NSObject(self=0x00000000014591c8, _cmd="P\x04", an
Object=0x000000000148f568, aKey=0x00007ffff62cc560) setValue:forKey:] + 382 at NSKeyValueCoding.m:370
    frame #8: 0x00007ffff5a2881d libgnustep-base.so.1.25`-[GSKVOBase setValue:forKey:](self=0x00000000014591c
8, _cmd="P\x04", anObject=0x000000000148f568, aKey=0x00007ffff62cc560) + 221 at NSKeyValueObserving.m:238
    frame #9: 0x00007ffff60a22ac libQuartzCore.so.0`-[CALayer init](self=0x00000000014591c8, _cmd="\xffffffa1

I added a debug statement to GSObjCSetVal:

          case _C_STRUCT_B:
            if (GSSelectorTypesMatch(@encode(NSPoint), type))
              {
                NSLog(@"match! point is %s, type is %s", @encode(NSPoint), type);
                NSPoint v = [val pointValue];

This is the output:
2018-05-27 17:58:46.980 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSPoint=dd}
2018-05-27 17:58:46.981 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSSize=dd}

Therefore, it's a bug in -base that makes it interpret sizes as points!

I'm still coming up with a minimum repro case.

(n.b. some of the confusion on why CGSize and CGPoint get understood as their NS equivalents is this chunk from opal:
  typedef NSPoint CGPoint;
  typedef NSSize CGSize;
  typedef NSRect CGRect;
I do not believe this to be correct, but I will not be addressing it at this time.

Separately: are typedefs of structs meant to be encoded as the original struct name? Probably yes?)


On Sun, May 27, 2018 at 5:29 PM Ivan Vučica <[hidden email]> wrote:
I added a test to the -base from May 20:

  NSPoint point = {.x = 16.0, .y = 32.0};
  NSValue *pointV = [NSValue valueWithPoint: point];

  result = !strcmp(@encode(NSPoint), [pointV objCType]);
  PASS(result, "@encoding(NSPoint) == pointV objCType");

  result = strcmp(@encode(NSSize), [pointV objCType]);
  PASS(result, "@encoding(NSSize) == pointV objCType");

with this temporarly ine:
  NSLog(@"%s %s %s", @encode(NSPoint), @encode(NSSize), [pointV objCType]);

It passes and prints out:

2018-05-27 17:27:38.823 size-point-encoding[25395:25395] {_NSPoint=dd} {_NSSize=dd} {_NSPoint=dd}

I''m annoyed at how I did not notice that I did use CGSize. I will introduce support for NSSize/CGSize.

Thanks!

On Mon, May 21, 2018 at 12:53 AM Fred Kiefer <[hidden email]> wrote:
I added a bit of code in base that allows to use NSValue objects for size and point with methods for the other type. This is a bit closer to the Cocoa behaviour but will require more tweaking to have it fully correct. It will at least allow you to run this test code again.

> Am 20.05.2018 um 16:02 schrieb Fred Kiefer <[hidden email]>:
>
> I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)
>
> The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
> In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.
>
> But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.
>
> Hope this helps,
> Fred
>
>> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
>>
>> Hi,
>>
>> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
>>
>> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
>>
>> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
>>
>> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
>>
>> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
>>
>> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
>>
>> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
>> - Is there a way out?
>> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
>>
>> Thanks
>> _______________________________________________
>> Gnustep-dev mailing list
>> [hidden email]
>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>
>
> _______________________________________________
> Gnustep-dev mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2
I'll open a bug for -base. In the meantime I'll help Stjepan disable shadowOffset.

Minimum repro::

#import <Foundation/Foundation.h>

@interface A : NSObject
{
  NSSize _s;
}
@end
@implementation A
- (NSSize) s {
  return self->_s;
}
- (void) setS: (NSSize) s {
  self->_s = s;
}
@end
int main() {
  NSSize in = NSMakeSize(1.0, 2.0);
  NSValue * v = [NSValue valueWithBytes: &in
                               objCType: @encode(NSSize)];
  A * a = [A new];
  [a setValue: v
       forKey: @"s"];


  return 0;
}


Output:
$ clang `gnustep-config --objc-flags` `gnustep-config --objc-libs` `gnustep-config --base-libs` repro.m -o repro && ./repro
clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
2018-05-27 18:42:13.137 repro[14432:14432] match! point is {_NSPoint=dd}, type is {_NSSize=dd}
./repro: Uncaught exception NSInvalidArgumentException, reason: [GSSizeValue-pointValue] should be overridden by subclass
Aborted





On Sun, May 27, 2018 at 6:18 PM Ivan Vučica <[hidden email]> wrote:
So turns out we don't use shadowOffset in any demo code. So examining CAAnimation was pointless.

However I managed to get a backtrace using lldb, and this happens when the code sets the default shadowOffset value. So you pointed to the right place; shadowOffset is the problem. This is the place of the exception, in -[CALayer init] which calls +[CALayer defaultValueForKey:]:

  if ([key isEqualToString: @"shadowOffset"])                                                                
    {
      CGSize offset = CGSizeMake(0.0, -3.0);
      return [NSValue valueWithBytes: &offset objCType: @encode(CGSize)];
    }

What is strange is that this happens deep inside base, when I am setting valueWithBytes, with a CGSize type.

So I looked closer at the backtrace and it's happening in base/Source/Additions/GSObjCRuntime.m. Relevant chunk:

    frame #2: 0x00007ffff59e9d7d libgnustep-base.so.1.25`+[NSException raise:format:](self=0x00007ffff5f69878
, _cmd="S", name=0x00007ffff5f69438, format=0x00007ffff6003828) + 365 at NSException.m:1376
    frame #3: 0x00007ffff5b9ca2f libgnustep-base.so.1.25`-[NSObject(self=0x000000000148f568, _cmd="\xffffffa9
\x02", aSel="\x0e\x01") subclassResponsibility:] + 255 at NSObject+GNUstepBase.m:134
    frame #4: 0x00007ffff5b1d32b libgnustep-base.so.1.25`-[NSValue pointValue](self=0x000000000148f568, _cmd=
"\x0e\x01") + 43 at NSValue.m:394
    frame #5: 0x00007ffff5b58649 libgnustep-base.so.1.25`GSObjCSetVal(self=0x00000000014591c8, key="shadowOff
set", val=0x000000000148f568, sel="\xffffffda\e", type="{_NSSize=dd}", size=12, offset=0) + 3497 at GSObjCRun
time.m:1794
    frame #6: 0x00007ffff5a25857 libgnustep-base.so.1.25`SetValueForKey(self=0x00000000014591c8, anObject=0x0
00000000148f568, key="shadowOffset", size=12) + 1079 at NSKeyValueCoding.m:150
    frame #7: 0x00007ffff5a253ee libgnustep-base.so.1.25`-[NSObject(self=0x00000000014591c8, _cmd="P\x04", an
Object=0x000000000148f568, aKey=0x00007ffff62cc560) setValue:forKey:] + 382 at NSKeyValueCoding.m:370
    frame #8: 0x00007ffff5a2881d libgnustep-base.so.1.25`-[GSKVOBase setValue:forKey:](self=0x00000000014591c
8, _cmd="P\x04", anObject=0x000000000148f568, aKey=0x00007ffff62cc560) + 221 at NSKeyValueObserving.m:238
    frame #9: 0x00007ffff60a22ac libQuartzCore.so.0`-[CALayer init](self=0x00000000014591c8, _cmd="\xffffffa1

I added a debug statement to GSObjCSetVal:

          case _C_STRUCT_B:
            if (GSSelectorTypesMatch(@encode(NSPoint), type))
              {
                NSLog(@"match! point is %s, type is %s", @encode(NSPoint), type);
                NSPoint v = [val pointValue];

This is the output:
2018-05-27 17:58:46.980 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSPoint=dd}
2018-05-27 17:58:46.981 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSSize=dd}

Therefore, it's a bug in -base that makes it interpret sizes as points!

I'm still coming up with a minimum repro case.

(n.b. some of the confusion on why CGSize and CGPoint get understood as their NS equivalents is this chunk from opal:
  typedef NSPoint CGPoint;
  typedef NSSize CGSize;
  typedef NSRect CGRect;
I do not believe this to be correct, but I will not be addressing it at this time.

Separately: are typedefs of structs meant to be encoded as the original struct name? Probably yes?)


On Sun, May 27, 2018 at 5:29 PM Ivan Vučica <[hidden email]> wrote:
I added a test to the -base from May 20:

  NSPoint point = {.x = 16.0, .y = 32.0};
  NSValue *pointV = [NSValue valueWithPoint: point];

  result = !strcmp(@encode(NSPoint), [pointV objCType]);
  PASS(result, "@encoding(NSPoint) == pointV objCType");

  result = strcmp(@encode(NSSize), [pointV objCType]);
  PASS(result, "@encoding(NSSize) == pointV objCType");

with this temporarly ine:
  NSLog(@"%s %s %s", @encode(NSPoint), @encode(NSSize), [pointV objCType]);

It passes and prints out:

2018-05-27 17:27:38.823 size-point-encoding[25395:25395] {_NSPoint=dd} {_NSSize=dd} {_NSPoint=dd}

I''m annoyed at how I did not notice that I did use CGSize. I will introduce support for NSSize/CGSize.

Thanks!

On Mon, May 21, 2018 at 12:53 AM Fred Kiefer <[hidden email]> wrote:
I added a bit of code in base that allows to use NSValue objects for size and point with methods for the other type. This is a bit closer to the Cocoa behaviour but will require more tweaking to have it fully correct. It will at least allow you to run this test code again.

> Am 20.05.2018 um 16:02 schrieb Fred Kiefer <[hidden email]>:
>
> I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)
>
> The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
> In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.
>
> But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.
>
> Hope this helps,
> Fred
>
>> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
>>
>> Hi,
>>
>> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
>>
>> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
>>
>> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
>>
>> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
>>
>> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
>>
>> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
>>
>> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
>> - Is there a way out?
>> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
>>
>> Thanks
>> _______________________________________________
>> Gnustep-dev mailing list
>> [hidden email]
>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>
>
> _______________________________________________
> Gnustep-dev mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2

On Sun, May 27, 2018 at 6:44 PM Ivan Vučica <[hidden email]> wrote:
I'll open a bug for -base. In the meantime I'll help Stjepan disable shadowOffset.

Minimum repro::

#import <Foundation/Foundation.h>

@interface A : NSObject
{
  NSSize _s;
}
@end
@implementation A
- (NSSize) s {
  return self->_s;
}
- (void) setS: (NSSize) s {
  self->_s = s;
}
@end
int main() {
  NSSize in = NSMakeSize(1.0, 2.0);
  NSValue * v = [NSValue valueWithBytes: &in
                               objCType: @encode(NSSize)];
  A * a = [A new];
  [a setValue: v
       forKey: @"s"];


  return 0;
}


Output:
$ clang `gnustep-config --objc-flags` `gnustep-config --objc-libs` `gnustep-config --base-libs` repro.m -o repro && ./repro
clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
2018-05-27 18:42:13.137 repro[14432:14432] match! point is {_NSPoint=dd}, type is {_NSSize=dd}
./repro: Uncaught exception NSInvalidArgumentException, reason: [GSSizeValue-pointValue] should be overridden by subclass
Aborted





On Sun, May 27, 2018 at 6:18 PM Ivan Vučica <[hidden email]> wrote:
So turns out we don't use shadowOffset in any demo code. So examining CAAnimation was pointless.

However I managed to get a backtrace using lldb, and this happens when the code sets the default shadowOffset value. So you pointed to the right place; shadowOffset is the problem. This is the place of the exception, in -[CALayer init] which calls +[CALayer defaultValueForKey:]:

  if ([key isEqualToString: @"shadowOffset"])                                                                
    {
      CGSize offset = CGSizeMake(0.0, -3.0);
      return [NSValue valueWithBytes: &offset objCType: @encode(CGSize)];
    }

What is strange is that this happens deep inside base, when I am setting valueWithBytes, with a CGSize type.

So I looked closer at the backtrace and it's happening in base/Source/Additions/GSObjCRuntime.m. Relevant chunk:

    frame #2: 0x00007ffff59e9d7d libgnustep-base.so.1.25`+[NSException raise:format:](self=0x00007ffff5f69878
, _cmd="S", name=0x00007ffff5f69438, format=0x00007ffff6003828) + 365 at NSException.m:1376
    frame #3: 0x00007ffff5b9ca2f libgnustep-base.so.1.25`-[NSObject(self=0x000000000148f568, _cmd="\xffffffa9
\x02", aSel="\x0e\x01") subclassResponsibility:] + 255 at NSObject+GNUstepBase.m:134
    frame #4: 0x00007ffff5b1d32b libgnustep-base.so.1.25`-[NSValue pointValue](self=0x000000000148f568, _cmd=
"\x0e\x01") + 43 at NSValue.m:394
    frame #5: 0x00007ffff5b58649 libgnustep-base.so.1.25`GSObjCSetVal(self=0x00000000014591c8, key="shadowOff
set", val=0x000000000148f568, sel="\xffffffda\e", type="{_NSSize=dd}", size=12, offset=0) + 3497 at GSObjCRun
time.m:1794
    frame #6: 0x00007ffff5a25857 libgnustep-base.so.1.25`SetValueForKey(self=0x00000000014591c8, anObject=0x0
00000000148f568, key="shadowOffset", size=12) + 1079 at NSKeyValueCoding.m:150
    frame #7: 0x00007ffff5a253ee libgnustep-base.so.1.25`-[NSObject(self=0x00000000014591c8, _cmd="P\x04", an
Object=0x000000000148f568, aKey=0x00007ffff62cc560) setValue:forKey:] + 382 at NSKeyValueCoding.m:370
    frame #8: 0x00007ffff5a2881d libgnustep-base.so.1.25`-[GSKVOBase setValue:forKey:](self=0x00000000014591c
8, _cmd="P\x04", anObject=0x000000000148f568, aKey=0x00007ffff62cc560) + 221 at NSKeyValueObserving.m:238
    frame #9: 0x00007ffff60a22ac libQuartzCore.so.0`-[CALayer init](self=0x00000000014591c8, _cmd="\xffffffa1

I added a debug statement to GSObjCSetVal:

          case _C_STRUCT_B:
            if (GSSelectorTypesMatch(@encode(NSPoint), type))
              {
                NSLog(@"match! point is %s, type is %s", @encode(NSPoint), type);
                NSPoint v = [val pointValue];

This is the output:
2018-05-27 17:58:46.980 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSPoint=dd}
2018-05-27 17:58:46.981 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSSize=dd}

Therefore, it's a bug in -base that makes it interpret sizes as points!

I'm still coming up with a minimum repro case.

(n.b. some of the confusion on why CGSize and CGPoint get understood as their NS equivalents is this chunk from opal:
  typedef NSPoint CGPoint;
  typedef NSSize CGSize;
  typedef NSRect CGRect;
I do not believe this to be correct, but I will not be addressing it at this time.

Separately: are typedefs of structs meant to be encoded as the original struct name? Probably yes?)


On Sun, May 27, 2018 at 5:29 PM Ivan Vučica <[hidden email]> wrote:
I added a test to the -base from May 20:

  NSPoint point = {.x = 16.0, .y = 32.0};
  NSValue *pointV = [NSValue valueWithPoint: point];

  result = !strcmp(@encode(NSPoint), [pointV objCType]);
  PASS(result, "@encoding(NSPoint) == pointV objCType");

  result = strcmp(@encode(NSSize), [pointV objCType]);
  PASS(result, "@encoding(NSSize) == pointV objCType");

with this temporarly ine:
  NSLog(@"%s %s %s", @encode(NSPoint), @encode(NSSize), [pointV objCType]);

It passes and prints out:

2018-05-27 17:27:38.823 size-point-encoding[25395:25395] {_NSPoint=dd} {_NSSize=dd} {_NSPoint=dd}

I''m annoyed at how I did not notice that I did use CGSize. I will introduce support for NSSize/CGSize.

Thanks!

On Mon, May 21, 2018 at 12:53 AM Fred Kiefer <[hidden email]> wrote:
I added a bit of code in base that allows to use NSValue objects for size and point with methods for the other type. This is a bit closer to the Cocoa behaviour but will require more tweaking to have it fully correct. It will at least allow you to run this test code again.

> Am 20.05.2018 um 16:02 schrieb Fred Kiefer <[hidden email]>:
>
> I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)
>
> The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
> In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.
>
> But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.
>
> Hope this helps,
> Fred
>
>> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
>>
>> Hi,
>>
>> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
>>
>> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
>>
>> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
>>
>> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
>>
>> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
>>
>> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
>>
>> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
>> - Is there a way out?
>> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
>>
>> Thanks
>> _______________________________________________
>> Gnustep-dev mailing list
>> [hidden email]
>> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>
>
> _______________________________________________
> Gnustep-dev mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Fred Kiefer
In reply to this post by Ivan Vučica-2
Hi Ivan,

great analysis and nice code to reproduce this, but you seem to have missed my mail on the issue (even though it is included in your mail) and the work around I added to base. I was already fully aware of this and the patch I added should prevent your test code from throwing the exception. Of course this is not the real solution. We may have to add the same checks for known types in GSObjCSetVal that we already have in other places. This is rather ugly code which I would prefer to avoid. I hope to see Richard in two weeks time and we may come up with a better solution together.

If the issue persists even with my work around in place it would be good to know. I have a few ideas for better work arounds but would prefer a cleaner solution.

Fred

PS: In your test code the second PASS is missing a „!“.

> Am 27.05.2018 um 19:44 schrieb Ivan Vučica <[hidden email]>:
>
> I'll open a bug for -base. In the meantime I'll help Stjepan disable shadowOffset.
>
> Minimum repro::
>
> #import <Foundation/Foundation.h>
>
> @interface A : NSObject
> {
>   NSSize _s;
> }
> @end
> @implementation A
> - (NSSize) s {
>   return self->_s;
> }
> - (void) setS: (NSSize) s {
>   self->_s = s;
> }
> @end
> int main() {
>   NSSize in = NSMakeSize(1.0, 2.0);
>   NSValue * v = [NSValue valueWithBytes: &in
>                                objCType: @encode(NSSize)];
>   A * a = [A new];
>   [a setValue: v
>        forKey: @"s"];
>
>
>   return 0;
> }
>
>
> Output:
> $ clang `gnustep-config --objc-flags` `gnustep-config --objc-libs` `gnustep-config --base-libs` repro.m -o repro && ./repro
> clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
> clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
> 2018-05-27 18:42:13.137 repro[14432:14432] match! point is {_NSPoint=dd}, type is {_NSSize=dd}
> ./repro: Uncaught exception NSInvalidArgumentException, reason: [GSSizeValue-pointValue] should be overridden by subclass
> Aborted
>
>
>
>
>
> On Sun, May 27, 2018 at 6:18 PM Ivan Vučica <[hidden email]> wrote:
> So turns out we don't use shadowOffset in any demo code. So examining CAAnimation was pointless.
>
> However I managed to get a backtrace using lldb, and this happens when the code sets the default shadowOffset value. So you pointed to the right place; shadowOffset is the problem. This is the place of the exception, in -[CALayer init] which calls +[CALayer defaultValueForKey:]:
>
>   if ([key isEqualToString: @"shadowOffset"])                                                                
>     {
>       CGSize offset = CGSizeMake(0.0, -3.0);
>       return [NSValue valueWithBytes: &offset objCType: @encode(CGSize)];
>     }
>
> What is strange is that this happens deep inside base, when I am setting valueWithBytes, with a CGSize type.
>
> So I looked closer at the backtrace and it's happening in base/Source/Additions/GSObjCRuntime.m. Relevant chunk:
>
>     frame #2: 0x00007ffff59e9d7d libgnustep-base.so.1.25`+[NSException raise:format:](self=0x00007ffff5f69878
> , _cmd="S", name=0x00007ffff5f69438, format=0x00007ffff6003828) + 365 at NSException.m:1376
>     frame #3: 0x00007ffff5b9ca2f libgnustep-base.so.1.25`-[NSObject(self=0x000000000148f568, _cmd="\xffffffa9
> \x02", aSel="\x0e\x01") subclassResponsibility:] + 255 at NSObject+GNUstepBase.m:134
>     frame #4: 0x00007ffff5b1d32b libgnustep-base.so.1.25`-[NSValue pointValue](self=0x000000000148f568, _cmd=
> "\x0e\x01") + 43 at NSValue.m:394
>     frame #5: 0x00007ffff5b58649 libgnustep-base.so.1.25`GSObjCSetVal(self=0x00000000014591c8, key="shadowOff
> set", val=0x000000000148f568, sel="\xffffffda\e", type="{_NSSize=dd}", size=12, offset=0) + 3497 at GSObjCRun
> time.m:1794
>     frame #6: 0x00007ffff5a25857 libgnustep-base.so.1.25`SetValueForKey(self=0x00000000014591c8, anObject=0x0
> 00000000148f568, key="shadowOffset", size=12) + 1079 at NSKeyValueCoding.m:150
>     frame #7: 0x00007ffff5a253ee libgnustep-base.so.1.25`-[NSObject(self=0x00000000014591c8, _cmd="P\x04", an
> Object=0x000000000148f568, aKey=0x00007ffff62cc560) setValue:forKey:] + 382 at NSKeyValueCoding.m:370
>     frame #8: 0x00007ffff5a2881d libgnustep-base.so.1.25`-[GSKVOBase setValue:forKey:](self=0x00000000014591c
> 8, _cmd="P\x04", anObject=0x000000000148f568, aKey=0x00007ffff62cc560) + 221 at NSKeyValueObserving.m:238
>     frame #9: 0x00007ffff60a22ac libQuartzCore.so.0`-[CALayer init](self=0x00000000014591c8, _cmd="\xffffffa1
>
> I added a debug statement to GSObjCSetVal:
>
>           case _C_STRUCT_B:
>             if (GSSelectorTypesMatch(@encode(NSPoint), type))
>               {
>                 NSLog(@"match! point is %s, type is %s", @encode(NSPoint), type);
>                 NSPoint v = [val pointValue];
>
> This is the output:
> 2018-05-27 17:58:46.980 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSPoint=dd}
> 2018-05-27 17:58:46.981 hello_carenderer[10274:10274] match! point is {_NSPoint=dd}, type is {_NSSize=dd}
>
> Therefore, it's a bug in -base that makes it interpret sizes as points!
>
> I'm still coming up with a minimum repro case.
>
> (n.b. some of the confusion on why CGSize and CGPoint get understood as their NS equivalents is this chunk from opal:
>   typedef NSPoint CGPoint;
>   typedef NSSize CGSize;
>   typedef NSRect CGRect;
> I do not believe this to be correct, but I will not be addressing it at this time.
>
> Separately: are typedefs of structs meant to be encoded as the original struct name? Probably yes?)
>
>
> On Sun, May 27, 2018 at 5:29 PM Ivan Vučica <[hidden email]> wrote:
> I added a test to the -base from May 20:
>
>   NSPoint point = {.x = 16.0, .y = 32.0};
>   NSValue *pointV = [NSValue valueWithPoint: point];
>
>   result = !strcmp(@encode(NSPoint), [pointV objCType]);
>   PASS(result, "@encoding(NSPoint) == pointV objCType");
>
>   result = strcmp(@encode(NSSize), [pointV objCType]);
>   PASS(result, "@encoding(NSSize) == pointV objCType");
>
> with this temporarly ine:
>   NSLog(@"%s %s %s", @encode(NSPoint), @encode(NSSize), [pointV objCType]);
>
> It passes and prints out:
>
> 2018-05-27 17:27:38.823 size-point-encoding[25395:25395] {_NSPoint=dd} {_NSSize=dd} {_NSPoint=dd}
>
> I''m annoyed at how I did not notice that I did use CGSize. I will introduce support for NSSize/CGSize.
>
> Thanks!
>
> On Mon, May 21, 2018 at 12:53 AM Fred Kiefer <[hidden email]> wrote:
> I added a bit of code in base that allows to use NSValue objects for size and point with methods for the other type. This is a bit closer to the Cocoa behaviour but will require more tweaking to have it fully correct. It will at least allow you to run this test code again.
>
> > Am 20.05.2018 um 16:02 schrieb Fred Kiefer <[hidden email]>:
> >
> > I spend some time to understand this issue. But to be honest it took me more time to understand why this ever worked :-)
> >
> > The problem here is that base uses two different ways to provide concrete subclasses for NSValue. One being GSValue, which supports generic data and the other mechanism is to have a specific subclass for types like NSPoint and NSSize. For the later base only generates specific methods for that types. Resulting in different classes for NSPoint and NSSize, which are incompatible although they have the same underlying data size.
> > In your demo application that animation for shadowOffset tries to set the offset size with a suitable default value of class GSSizeValue. But the code in GSObjCSetVal only compares the type information without looking at the type names. That way NSSize is regarded as the same as NSPoint and the pointValue get executed, resulting in the error you reported. Either GSObjCSetVal has to be more careful and use a check for the actual type first. Or we need to make the NSValue subclasses more general.
> >
> > But why did it work before? Most likely because at that time CGSize and CGPoint, where different from NSSize and NSPoint so we did not get the specific optimisation in NSValue.
> >
> > Hope this helps,
> > Fred
> >
> >> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <[hidden email]>:
> >>
> >> Hi,
> >>
> >> Pretty much all Core Animation demos are currently broken under GNUstep with a variation on the following:
> >>
> >> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException REASON:[GSSizeValue-pointValue] should be overridden by subclass INFO:(null)
> >>
> >> CA is accessing -pointValue method if it determines that the passed NSValue matches the -objCType of NSPoint. It does not check for size values.
> >>
> >> Clearly, sometimes it is trying to interpolate size values, which will match the signature and it will incorrectly attempt to access -pointValue which is then not implemented by GSSizeValue. I am not sure where might it be interpolating size values, but it seems to be doing so. (Alternative bug is that NSValue is incorrectly ingesting points as sizes, then complaining when the point provided is being interpreted as a point. I can try checking this later.)
> >>
> >> I cannot check -respondsToSelector: because the class /does/ in fact respond to -pointValue; it just throws an exception.
> >>
> >> Adding a try/catch in this kind of situation would make for some very poor code hygiene, in my opinion.
> >>
> >> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this time, can't check.)
> >> - Is there a way out?
> >> - Would it make sense to extend GSSizeValue and add -pointValue to it? (They're both 2d vectors.)
> >>
> >> Thanks


_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2
On Sun, May 27, 2018 at 7:47 PM Fred Kiefer <[hidden email]> wrote:
Hi Ivan,

great analysis and nice code to reproduce this, but you seem to have missed my mail on the issue (even though it is included in your mail) and the work around I added to base.

I saw it, but it was not clear how deeply you've looked into it. That is why I mentioned that I use the revision from May 20. :-)
 
I was already fully aware of this and the patch I added should prevent your test code from throwing the exception.

It indeed has; after figuring out what the cause is, I wrote this to be able to check whether your fixes which you mentioned in the email work:

Test that no longer fails is setting the size using KVC.

I'm also pleased that I can probably remove "KVO notification does not get triggered for setting struct properties", as well -- I think you told me years ago that this was fixed, but only now did I check :-)

If you tell me this test is useful, I can open a PR and we can do a code review there.
 
Of course this is not the real solution. We may have to add the same checks for known types in GSObjCSetVal that we already have in other places. This is rather ugly code which I would prefer to avoid. I hope to see Richard in two weeks time and we may come up with a better solution together.

I am in favor of leaving the bug open on Savannah and Github, then? I can no longer reproduce the original issue after updating to the latest fixes, but if you believe the solution that's in place is hacky, maybe the bugs can be repurposed.

Otherwise, I am fine with them being closed (as the immediate NSSize/NSPoint problem no longer persists).

If the issue persists even with my work around in place it would be good to know. I have a few ideas for better work arounds but would prefer a cleaner solution.

Fred

PS: In your test code the second PASS is missing a „!“.

It doesn't; @encode(NSPoint) should be different than @encode(NSSize).

I actually added comments to the new test case I pushed to my personal repo, so it should be clearer what's going on.
 

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Fred Kiefer
Sorry, you seem to be using an unusual mail format that my Apple Mail isn’t able to properly reply to.

> Am 27.05.2018 um 21:17 schrieb Ivan Vučica <[hidden email]>:
> On Sun, May 27, 2018 at 7:47 PM Fred Kiefer <[hidden email]> wrote:
> I was already fully aware of this and the patch I added should prevent your test code from throwing the exception.
>
> It indeed has; after figuring out what the cause is, I wrote this to be able to check whether your fixes which you mentioned in the email work:
>   https://github.com/ivucica/libs-base/commit/4771a84f7fd4d8b1536b9f52ef30ec524ec6357d


Great.

> Test that no longer fails is setting the size using KVC.
>
> I'm also pleased that I can probably remove "KVO notification does not get triggered for setting struct properties", as well -- I think you told me years ago that this was fixed, but only now did I check :-)
>
> If you tell me this test is useful, I can open a PR and we can do a code review there.

Yes, we should keep it.
 
> Of course this is not the real solution. We may have to add the same checks for known types in GSObjCSetVal that we already have in other places. This is rather ugly code which I would prefer to avoid. I hope to see Richard in two weeks time and we may come up with a better solution together.
>
> I am in favor of leaving the bug open on Savannah and Github, then? I can no longer reproduce the original issue after updating to the latest fixes, but if you believe the solution that's in place is hacky, maybe the bugs can be repurposed.
>
> Otherwise, I am fine with them being closed (as the immediate NSSize/NSPoint problem no longer persists).

No, keep the two bug reports open. It might be that when we come up with a fix we will forget to close one of them. Please keep an eye open for that case and remind me.

> PS: In your test code the second PASS is missing a „!“.
>
> It doesn't; @encode(NSPoint) should be different than @encode(NSSize).
>
> I actually added comments to the new test case I pushed to my personal repo, so it should be clearer what's going on.

I was referring to this line:

 PASS(result, "@encoding(NSSize) == pointV objCType“);

And there you check that the two values are not equal, which in C like programming languages is done via a negation that is missing in the description string.

I really should be more verbose in my writing. The reason here is that I am no longer used to communications in English. Sorry for that.

Fred



_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
Reply | Threaded
Open this post in threaded view
|

Re: CA broken; how do I differentiate NSValues for size and point?

Ivan Vučica-2
Thanks for all the attention you've given to this!

On Sun, May 27, 2018 at 10:29 PM Fred Kiefer <[hidden email]> wrote:

> Sorry, you seem to be using an unusual mail format that my Apple Mail
isn’t able to properly reply to.

I hit reply-to-all in Gmail, then I hit enter where I want to inline reply.
I don't have insight into what Gmail does before sending.

I'll switch to plaintext only (which I wanted to use anyway), which should
help.

> > I am in favor of leaving the bug open on Savannah and Github, then? I
can no longer reproduce the original issue after updating to the latest
fixes, but if you believe the solution that's in place is hacky, maybe the
bugs can be repurposed.
> >
> > Otherwise, I am fine with them being closed (as the immediate
NSSize/NSPoint problem no longer persists).

> No, keep the two bug reports open. It might be that when we come up with
a fix we will forget to close one of them. Please keep an eye open for that
case and remind me.

Ah -- it's unlikely I'll remember something you won't :-)

Maybe you can assign the bug(s) to yourself, so they appear in bug
dashboards?


> > PS: In your test code the second PASS is missing a „!“.
> >
> > It doesn't; @encode(NSPoint) should be different than @encode(NSSize).
> >
> > I actually added comments to the new test case I pushed to my personal
repo, so it should be clearer what's going on.

> I was referring to this line:

>   PASS(result, "@encoding(NSSize) == pointV objCType“);

> And there you check that the two values are not equal, which in C like
programming languages is done via a negation that is missing in the
description string.

Oh, right. Sorry. That was an oversight which I corrected after sending the
email. :-)

> I really should be more verbose in my writing. The reason here is that I
am no longer used to communications in English. Sorry for that.

I hope to be in that situation again someday ;-)

_______________________________________________
Gnustep-dev mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/gnustep-dev