[Lazarus] Breaking change

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

[Lazarus] Breaking change

Free Pascal - Lazarus mailing list
Hello,

I have fixed bug 28760:

https://bugs.freepascal.org/view.php?id=28760

This will cause compilation of win32wsdialogs.pp to fail, in particular in

class procedure TWin32WSOpenDialog.SetupVistaFileDialog(ADialog: IFileDialog; const AOpenDialog: TOpenDialog);

search for:

   if InitialDir <> '' then
   begin
     if Succeeded(SHCreateItemFromParsingName(PWideChar(UTF8ToUTF16(InitialDir)), nil, IShellItem, DefaultFolderItem)) then
       ADialog.SetFolder(DefaultFolderItem);
   end;

Directly passing an interface where (T)REFIID is expected, will no longer be possible.

Michael.
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
Hello,

> I have fixed bug 28760:
>
> https://bugs.freepascal.org/view.php?id=28760
as the reporter: not really.

The pointer-ness of REFIID is an artefact of the C-ABI. It is not meant to mean
'pass a pointer to a GUID-struct', but 'pass a GUID using byref'. We have
constref for that.

> Directly passing an interface where (T)REFIID is expected, will no longer be
possible.
It must be, if only for Delphi compat.

Delphi import is (... const riid: TIID;...), but that only works because they
never seem to pass structured types on the stack, regardless of size.


Martok

--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list


On Sun, 23 Jul 2017, Martok via Lazarus wrote:

> Hello,
>
>> I have fixed bug 28760:
>>
>> https://bugs.freepascal.org/view.php?id=28760
> as the reporter: not really.
>
> The pointer-ness of REFIID is an artefact of the C-ABI. It is not meant to mean
> 'pass a pointer to a GUID-struct', but 'pass a GUID using byref'. We have
> constref for that.

See the link to the Windows MSDN.

Michael.
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
Am 23.07.2017 um 14:24 schrieb Michael Van Canneyt via Lazarus:
>> The pointer-ness of REFIID is an artefact of the C-ABI. It is not meant to mean
>> 'pass a pointer to a GUID-struct', but 'pass a GUID using byref'. We have
>> constref for that.
>
> See the link to the Windows MSDN.
Yes, so?

REFIID is also the type of the first argument of QueryInterface.
<https://msdn.microsoft.com/en-us/library/windows/desktop/ms682521%28v=vs.85%29.aspx>

    { IUnknown }
    function QueryInterface({$IFDEF
FPC_HAS_CONSTREF}constref{$ELSE}const{$ENDIF} IID: TGUID; out Obj): Hresult;
virtual; {$IFNDEF WINDOWS}cdecl{$ELSE}stdcall{$ENDIF};

But I see already it has been translated differently in every unit (mostly
because MS made it uniform only after the headers were translated), so might as
well use the new one to keep things interesting. Carry on.

--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list


On Sun, 23 Jul 2017, Martok via Lazarus wrote:

> Am 23.07.2017 um 14:24 schrieb Michael Van Canneyt via Lazarus:
>>> The pointer-ness of REFIID is an artefact of the C-ABI. It is not meant to mean
>>> 'pass a pointer to a GUID-struct', but 'pass a GUID using byref'. We have
>>> constref for that.
>>
>> See the link to the Windows MSDN.
> Yes, so?

Well, if they declare it as a pointer, why should I not do the same :) ?

>
> REFIID is also the type of the first argument of QueryInterface.
> <https://msdn.microsoft.com/en-us/library/windows/desktop/ms682521%28v=vs.85%29.aspx>

I know.

>
>    { IUnknown }
>    function QueryInterface({$IFDEF
> FPC_HAS_CONSTREF}constref{$ELSE}const{$ENDIF} IID: TGUID; out Obj): Hresult;
> virtual; {$IFNDEF WINDOWS}cdecl{$ELSE}stdcall{$ENDIF};

That is a historical monstrosity :)

>
> But I see already it has been translated differently in every unit (mostly
> because MS made it uniform only after the headers were translated), so might as
> well use the new one to keep things interesting. Carry on.

I agree the matter is a bit dubious :/

But, before doing anything, I looked around. Saw some C# and VB import
statements. Noticed even in Delphi it's not 100% clear (see winrt* units),
and finally decided to do it like this to make people aware of the fact
that if they used a refiid argument in the past, it can hardly have worked.
The current definition makes it quite clear that it is in fact a pointer to a GUID.

Michael.
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list

>>    { IUnknown }
>>    function QueryInterface({$IFDEF
>> FPC_HAS_CONSTREF}constref{$ELSE}const{$ENDIF} IID: TGUID; out Obj): Hresult;
>> virtual; {$IFNDEF WINDOWS}cdecl{$ELSE}stdcall{$ENDIF};
>
> That is a historical monstrosity :)
I a gree we can do away with the IFDEFS and just use constref.

>> But I see already it has been translated differently in every unit (mostly
>> because MS made it uniform only after the headers were translated), so might as
>> well use the new one to keep things interesting. Carry on.
>
> I agree the matter is a bit dubious :/
I think it's really not. We can transparently use an interface name where a GUID
is required for a reason, and that is part of why it's easier to write COM
applications in Delphi than in MS compilers.
Were your new definition applied globally and none of the APIs actually use the
non-pointer type, we might as well get rid of that feature, as it'd be useless then.
> The current definition makes it quite clear that it is in fact a pointer to a GUID.
In MSDN hungarian notation terms, it is very obviously not: REFIID is
notationally different from LPIID (or just plain *IID in argument lists).



--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list


On Sun, 23 Jul 2017, Martok via Lazarus wrote:

>
>>>    { IUnknown }
>>>    function QueryInterface({$IFDEF
>>> FPC_HAS_CONSTREF}constref{$ELSE}const{$ENDIF} IID: TGUID; out Obj): Hresult;
>>> virtual; {$IFNDEF WINDOWS}cdecl{$ELSE}stdcall{$ENDIF};
>>
>> That is a historical monstrosity :)
> I a gree we can do away with the IFDEFS and just use constref.
>
>>> But I see already it has been translated differently in every unit (mostly
>>> because MS made it uniform only after the headers were translated), so might as
>>> well use the new one to keep things interesting. Carry on.
>>
>> I agree the matter is a bit dubious :/
> I think it's really not. We can transparently use an interface name where a GUID
> is required for a reason, and that is part of why it's easier to write COM
> applications in Delphi than in MS compilers.
> Were your new definition applied globally and none of the APIs actually use the
> non-pointer type, we might as well get rid of that feature, as it'd be useless then.
>> The current definition makes it quite clear that it is in fact a pointer to a GUID.
> In MSDN hungarian notation terms, it is very obviously not: REFIID is
> notationally different from LPIID (or just plain *IID in argument lists).

Well, I prefer something less ambiguous, not relying on murky ABI agreements.
So I acted accordingly.


Michael.
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
In reply to this post by Free Pascal - Lazarus mailing list
On 23.07.2017 12:50, Michael Van Canneyt via Lazarus wrote:
> Directly passing an interface where (T)REFIID is expected, will no
> longer be possible.

What is the fix?

"@GetTypeData(TypeInfo(IShellItem))^.Guid" ?

Ondrej
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list


On Sun, 23 Jul 2017, Ondrej Pokorny via Lazarus wrote:

> On 23.07.2017 12:50, Michael Van Canneyt via Lazarus wrote:
>> Directly passing an interface where (T)REFIID is expected, will no
>> longer be possible.
>
> What is the fix?
>
> "@GetTypeData(TypeInfo(IShellItem))^.Guid" ?

That is one possible fix, yes.

Maybe we can add a leightweight function for this to the typinfo unit.

Michael.
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
On 23.07.2017 18:11, Michael Van Canneyt via Lazarus wrote:
> That is one possible fix, yes.
>
> Maybe we can add a leightweight function for this to the typinfo unit.

Actually it works well with a local variable. I fixed it in r55562. I
hope it's fine so.

Ondrej
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
In reply to this post by Free Pascal - Lazarus mailing list
On 23.07.2017 12:50, Michael Van Canneyt via Lazarus wrote:

> Hello,
>
> I have fixed bug 28760:
>
> https://bugs.freepascal.org/view.php?id=28760
>
> This will cause compilation of win32wsdialogs.pp to fail, in particular in
>
> class procedure TWin32WSOpenDialog.SetupVistaFileDialog(ADialog:
> IFileDialog; const AOpenDialog: TOpenDialog);
>
> search for:
>
>   if InitialDir <> '' then
>   begin
>     if
> Succeeded(SHCreateItemFromParsingName(PWideChar(UTF8ToUTF16(InitialDir)), nil,
> IShellItem, DefaultFolderItem)) then
>       ADialog.SetFolder(DefaultFolderItem);
>   end;
>
> Directly passing an interface where (T)REFIID is expected, will no
> longer be possible.

I don't think that it was necessary to change (T)REFIID to be a pointer
and thus making COM interface usage unnecessarily more complicated in
Object Pascal:
- Microsoft implicitely defines a REFIID in guiddef.h to always be an
"IN" parameter, by defining it as "const IID &" for C++ code, so it can
never be written to anyway, thus a "constref" would have sufficed (a
"const IID &" in C++ behaves the same as a "IID *" in C or a "constref
x: IID" in FPC)
- aggregates (which is what a TGUID is) are always passed as pointers
for stdcall functions (basically all functions that are related to COM
and thus to REFIID) and also the Win64 ABI anyway, so a mere "TIID"
already behaved correctly for the Windows targets

Regards,
Sven
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list


On Sun, 23 Jul 2017, Sven Barth via Lazarus wrote:

> On 23.07.2017 12:50, Michael Van Canneyt via Lazarus wrote:
>> Hello,
>>
>> I have fixed bug 28760:
>>
>> https://bugs.freepascal.org/view.php?id=28760
>>
>> This will cause compilation of win32wsdialogs.pp to fail, in particular in
>>
>> class procedure TWin32WSOpenDialog.SetupVistaFileDialog(ADialog:
>> IFileDialog; const AOpenDialog: TOpenDialog);
>>
>> search for:
>>
>>   if InitialDir <> '' then
>>   begin
>>     if
>> Succeeded(SHCreateItemFromParsingName(PWideChar(UTF8ToUTF16(InitialDir)), nil,
>> IShellItem, DefaultFolderItem)) then
>>       ADialog.SetFolder(DefaultFolderItem);
>>   end;
>>
>> Directly passing an interface where (T)REFIID is expected, will no
>> longer be possible.
>
> I don't think that it was necessary to change (T)REFIID to be a pointer
> and thus making COM interface usage unnecessarily more complicated in
> Object Pascal:
> - Microsoft implicitely defines a REFIID in guiddef.h to always be an
> "IN" parameter, by defining it as "const IID &" for C++ code, so it can
> never be written to anyway, thus a "constref" would have sufficed (a
> "const IID &" in C++ behaves the same as a "IID *" in C or a "constref
> x: IID" in FPC)
> - aggregates (which is what a TGUID is) are always passed as pointers
> for stdcall functions (basically all functions that are related to COM
> and thus to REFIID) and also the Win64 ABI anyway, so a mere "TIID"
> already behaved correctly for the Windows targets

Being old school, I really don't like all this magic.
But if you feel it necessary, you are of course free to change it back.

Michael.
--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
In reply to this post by Free Pascal - Lazarus mailing list
Am 23.07.2017 um 22:01 schrieb Sven Barth via Lazarus:
> - aggregates (which is what a TGUID is) are always passed as pointers
> for stdcall functions (basically all functions that are related to COM
> and thus to REFIID) and also the Win64 ABI anyway, so a mere "TIID"
> already behaved correctly for the Windows targets
They weren't always, at least in this particular revision I reported the
original bug against. Ended up having the GUID itself on the stack, not a
reference - but only with const, and only in some functions. No specifier would
have worked too, but I think constref is more descriptive.

--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Lazarus] Breaking change

Free Pascal - Lazarus mailing list
In reply to this post by Free Pascal - Lazarus mailing list

Am 23.07.2017 12:50 schrieb "Michael Van Canneyt via Lazarus" <[hidden email]>:
>
> Hello,
>
> I have fixed bug 28760:
>
> https://bugs.freepascal.org/view.php?id=28760
>
> This will cause compilation of win32wsdialogs.pp to fail, in particular in
>
> class procedure TWin32WSOpenDialog.SetupVistaFileDialog(ADialog: IFileDialog; const AOpenDialog: TOpenDialog);
>
> search for:
>
>   if InitialDir <> '' then
>   begin
>     if Succeeded(SHCreateItemFromParsingName(PWideChar(UTF8ToUTF16(InitialDir)), nil, IShellItem, DefaultFolderItem)) then
>       ADialog.SetFolder(DefaultFolderItem);
>   end;
>
> Directly passing an interface where (T)REFIID is expected, will no longer be possible.

I reverted the changes by Michael and changed the parameters of those functions to constref instead.
However that doesn't mean that there won't be breaking changes: there are some interfaces that take REFIID (or TGuid parameters that are generally used with interface IIDs) and those will need to be changed as well. Most will be interface which have implementations provided by e.g. Windows, but in some cases those might be implemented by users and thus will lead to compilation failures (but at least those will be easy to spot :) ).

Regards,
Sven


--
_______________________________________________
Lazarus mailing list
[hidden email]
https://lists.lazarus-ide.org/listinfo/lazarus
Loading...