Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed length inline arrays are difficult to initialize #385

Closed
palenshus opened this issue Sep 1, 2021 · 9 comments · Fixed by #395
Closed

Fixed length inline arrays are difficult to initialize #385

palenshus opened this issue Sep 1, 2021 · 9 comments · Fixed by #395
Labels
enhancement New feature or request

Comments

@palenshus
Copy link

Actual behavior

When I gen code for the WER_REPORT_INFORMATION struct, the string fields are immutable:

			internal __char_64 wzConsentKey;
			/// <summary>The display name. If this member is empty, the default is the name specified by <i>pwzEventType</i> parameter of <a href="https://docs.microsoft.com/windows/desktop/api/werapi/nf-werapi-werreportcreate">WerReportCreate</a>.</summary>
			internal __char_128 wzFriendlyEventName;
			/// <summary>The name of the application. If this parameter is empty, the default is the base name of the image file.</summary>
			internal __char_128 wzApplicationName;
			/// <summary>The full path to the application.</summary>
			internal __char_260 wzApplicationPath;
			/// <summary>A description of the problem. This description is displayed in <b>Problem Reports and Solutions</b> on Windows Vista or the problem reports pane of the <b>Action Center</b> on Windows 7.</summary>
			internal __char_512 wzDescription;

image

Expected behavior

The string fields should be of type string, or perhaps char[N], but as they are now, there's no way for me to set them.

Repro steps

  1. NativeMethods.txt content:
WerReportCreate
  1. NativeMethods.json content (if present):
  1. Any of your own code that should be shared?
Windows.Win32.System.ErrorReporting.WER_REPORT_INFORMATION reportInformation = new();
reportInformation.wzDescription = "Hello world";

Context

  • CsWin32 version: 0.1.506-beta
  • Target Framework: .NET 6.0
@palenshus palenshus added the bug Something isn't working label Sep 1, 2021
@palenshus
Copy link
Author

It's quite possible I'm missing a way to set these values, but if so, I can't find it in the documentation. If so, maybe this could be a doc bug

@AArnott
Copy link
Member

AArnott commented Sep 2, 2021

We cannot use char[] or string as these are both reference types, and these char arrays are inline in the struct, which requires very particular C# syntax to express.

One way you can initialize this now is with:

WER_REPORT_INFORMATION wer = default;
string someValue = "some value";
someValue.AsSpan().CopyTo(wer.wzApplicationName.AsSpan());

That requires .NET Core. If you're on something less, it's significantly harder:

WER_REPORT_INFORMATION wer = default;
string someValue = "some value";
unsafe
{
    char* pwzAppName = &wer.wzApplicationName._0;
    for (int i = 0; i < someValue.Length && i < wer.wzApplicationName.Length; i++)
    {
        (*pwzAppName++) = someValue[i];
    }
}

I think when we wrote the helpers here we were focused on reading. I'll see if we can improve this, particularly on writing.

@AArnott AArnott added enhancement New feature or request and removed bug Something isn't working labels Sep 2, 2021
@AArnott
Copy link
Member

AArnott commented Sep 2, 2021

It looks like we can do substantially better. For writing we could make it as easy as assigning a string. Reading we could make it super-easy too. Something like this:

string someValue = "some value";
WER wer2;
wer2.wzApplicationName = someValue;
someValue = (string)wer2.wzApplicationName;

internal struct WER
{
    internal __char_64 wzApplicationName;
}

internal struct __char_64
{
	internal char _0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, _25, _26, _27, _28, _29, _30, _31, _32, _33, _34, _35, _36, _37, _38, _39, _40, _41, _42, _43, _44, _45, _46, _47, _48, _49, _50, _51, _52, _53, _54, _55, _56, _57, _58, _59, _60, _61, _62, _63;

	/// <summary>Always <c>64</c>.</summary>
	internal int Length => 64;

    public static implicit operator __char_64(string value)
    {
        __char_64 self = default;
        if (value.Length > self.Length)
        {
            throw new ArgumentException("Too long");
        }

        unsafe
        {
            char* pwzAppName = &self._0;
            for (int i = 0; i < value.Length; i++)
            {
                (*pwzAppName++) = value[i];
            }
        }

        return self;
    }

    public static explicit operator string(__char_64 value)
    {
        return new string(value._0, value.Length);
    }
}

@AArnott AArnott changed the title Input strings on struct are immutable Fixed length inline arrays are difficult to initialize Sep 3, 2021
@AArnott
Copy link
Member

AArnott commented Sep 3, 2021

Not all fixed arrays are strings. For those that are not based on char we should make sure our API allows for initializing them in a reasonable way as well.

@jnm2
Copy link
Contributor

jnm2 commented Sep 3, 2021

#301 will give a better solution for reading than the explicit string cast above, IMO.

@AArnott
Copy link
Member

AArnott commented Sep 3, 2021

Ah! I knew I'd seen it somewhere. I just couldn't remember where. I guess I need to get back to that PR. :)

@jnm2
Copy link
Contributor

jnm2 commented Sep 3, 2021

It's my PR... that I forgot about...

@palenshus
Copy link
Author

This is great, thanks @AArnott! I'll use the workaround above for now, and look forward being able to simplify my code if your proposed improvement makes it in, cheers!

@AArnott
Copy link
Member

AArnott commented Sep 15, 2021

The reading scenario PR is expected to merge today.
The init scenario will be tracked by this active issue and I hope to address it this week.

I hope we can address the utf-8 (byte) scenario as well as the utf-16 (char) one, but utf-8 may require more work and I'll file another issue if that's the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants