microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2.07k stars 87 forks source link

Windows.Win32.Foundation.FILETIME is not used in PROPVARIANT #835

Open harborsiem opened 1 year ago

harborsiem commented 1 year ago

Is your feature request related to a problem? Please describe. In the PROPVARIANT struct and in PInvoke.PropVariantGetFileTimeElem System.Runtime.InteropServices.ComTypes.FILETIME is used. I think, it may be a good idea to use the FILETIME from namespace Windows.Win32.Foundation. At the moment there is nearly no difference between the System.Runtime.InteropServices.ComTypes.FILETIME and Windows.Win32.Foundation.FILETIME. But in a later version of Windows.Win32.Foundation.FILETIME I expect converters to the .NET type System.DateTime like the code example.

using FT = System.Runtime.InteropServices.ComTypes.FILETIME;

namespace Windows.Win32.Foundation
{
    partial struct FILETIME
    {
        public FILETIME(DateTime dateTime)
        {
            long hFT = dateTime.ToFileTime();
            dwLowDateTime = (uint)(hFT & 0xFFFFFFFF);
            dwHighDateTime = (uint)((ulong)hFT >> 32);
        }

        public FILETIME(FT ft) { dwLowDateTime = (uint)ft.dwLowDateTime; dwHighDateTime = (uint)ft.dwHighDateTime; }

        public DateTime FromFileTime => DateTime.FromFileTime((long)(((ulong)dwHighDateTime) << 32) + dwLowDateTime);

        public FT ToComTypesFileTime
        {
            get
            {
                FT result = new FT();
                result.dwLowDateTime = (int)dwLowDateTime;
                result.dwHighDateTime = (int)dwHighDateTime;
                return result;
            }
        }
    }
}

Describe the solution you'd like Change the FILETIME in PROPVARIANT, PropVariantGetFileTimeElem, and in other functions (I don't know) and add the extended functions I describe.

Describe alternatives you've considered

Additional context

AArnott commented 1 year ago

CsWin32 already replaces all use of FILETIME with the struct defined in the BCL. It doesn't forbid generation of the struct from the metadata if you ask for it directly, but it otherwise doesn't generate it because there are no uses of it elsewhere in generated code. Why should we emit our own for use in PROPVARIANT?

It sounds like you're hoping to add methods to FILETIME in order to make it more useful than the BCL-defined struct. But given these are useful conversions, why not open an issue in the dotnet/runtime repo to ask the BCL to improve their existing struct?

harborsiem commented 1 year ago

In my opinion the FILETIME in the BCL is wrong designed, because there are Int32 fields for the dwLowDateTime, dwHighDateTime designed. CsWin32 does it correctly with UInt32 fields. So it is more intuitive for a user how to cast the values to a long that one have to use in DateTime.FromFileTime(long). Look at a small example how the casts can be done by a user:

            long max = DateTime.MaxValue.ToFileTime();
            int dwLow = -22;
            int dwHigh = 0x240fffff;
            long ft = ((long)dwHigh << 32) + dwLow;
            ulong ft1 = (ulong)dwHigh * 0x100000000 + (uint)dwLow;
            ulong ft2 = ((ulong)dwHigh << 32) + (uint)dwLow;
            long ft3 = (long)ft2;
            DateTime.FromFileTime((long)ft1);

if the dwLow value is a negative number then I got a wrong result with the value of "ft". And I think most users do it the wrong way and do not notice that they are wrong, because they test with a positive Int32. So the CsWin32 can help to avoid the wrong way. It's also a good idea to show in the documentation of the BCL the right way for combining the Int32 values to a Int64 value.

AArnott commented 1 year ago

@harborsiem: tip, please use 3 backticks around code blocks so that github properly formats them. By adding cs after the opening 3 backticks, you get syntax highlighting as well. I edited your issue description and last comment to fix the formatting of your messages, so you can switch to the edit view yourself to see how I did this.

You make good points. Perhaps we should go ahead with our own FILETIME struct and just add conversion operators to get along well with the BCL type.