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

Generate ToString methods on fixed-length char arrays #301

Merged
merged 7 commits into from
Sep 15, 2021

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Jun 15, 2021

Closes #281. I can drop the unused usings commit if it's not desired. It's been helpful in my projects and I thought it might help here.

TODO:

  • RS0030: The symbol 'InvocationExpressionSyntax.AddArgumentListArguments(params ArgumentSyntax[])' is banned in this project: Avoid implicit elastic trivia
  • Test functionality of ToString overrides
  • Improve whitespace

Generated syntax (updated):

				/// <summary>
				/// Copies the fixed array to a new string up to the specified length regardless of whether there are null terminating characters.
				/// </summary>
				/// <exception cref="ArgumentOutOfRangeException">
				/// Thrown when <paramref name="length"/> is less than <c>0</c> or greater than <see cref="Length"/>.
				/// </exception>
				internal unsafe string ToString(int length)
				{
if (length < 0 || length > Length)throw new ArgumentOutOfRangeException(nameof(length),length,"Length must be between 0 and the fixed array length.");
					fixed (char* p0 = &_0)
						return new string(p0,0,length);
				}

				/// <summary>
				/// Copies the fixed array to a new string, stopping before the first null terminator character or at the end of the fixed array (whichever is shorter).
				/// </summary>
				public override string ToString()
				{
var terminatorIndex = AsSpan().IndexOf(0);
					return ToString(terminatorIndex != -1 ? terminatorIndex : Length);
				}

@jnm2 jnm2 force-pushed the fixed_char_array_tostring branch 2 times, most recently from c33b557 to ae88117 Compare June 15, 2021 23:28
Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. It's mostly ready, I think. But a few changes are appropriate.

Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Show resolved Hide resolved
.editorconfig Show resolved Hide resolved
src/Microsoft.Windows.CsWin32/Generator.cs Outdated Show resolved Hide resolved
src/Microsoft.Windows.CsWin32/Generator.cs Outdated Show resolved Hide resolved
Comment on lines 775 to 790
/// <summary>
/// Copies the fixed array to a new string up to the specified length regardless of whether there are null terminating characters.
/// </summary>
/// <exception cref = ""ArgumentOutOfRangeException"">
/// Thrown when <paramref name = ""length""/> is less than <c>0</c> or greater than <see cref = ""Length""/>.
/// </exception>
internal string ToString(int length)
{
if (length < 0 || length > Length)
throw new ArgumentOutOfRangeException(nameof(length), length, ""Length must be between 0 and the fixed array length."");
unsafe
{
fixed (char *p0 = &_0)
return new string (p0, 0, length);
}
}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With #303, whitespace is becoming much more malleable. The few verbatim tests we have already were the very last that I got working. I don't want more of them, as we're likely to make additional whitespace changes in the future and I don't want the tax of matching this expectation or updating the tests. Please just assert the member exists, and if you want to test functionality, do that in the BasicTests.cs file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Testing the functionality is super important if the primary output (the source itself) is no longer tested, so I'll probably take a while on this. You didn't ask for input, so I won't belabor the point further.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 21, 2021

@AArnott I'm having some trouble that I don't know how to resolve. I deleted my clone and recloned from my fork and added the upstream remote and ran .\init.ps1, but ext\sdk-api is empty after this and the build fails.

Build started...
1>------ Build started: Project: ScrapeDocs, Configuration: Debug Any CPU ------
1>C:\Users\Joseph\.nuget\packages\microsoft.build.tasks.git\1.0.0\build\Microsoft.Build.Tasks.Git.targets(24,5): warning : Could not find file 'C:\Users\Joseph\Source\Repos\CsWin32\ext\sdk-api\.git'. The source code won't be available via Source Link.
1>ScrapeDocs -> C:\Users\Joseph\Source\Repos\CsWin32\bin\ScrapeDocs\Debug\net5.0\ScrapeDocs.dll
1>Done building project "ScrapeDocs.csproj".
2>------ Build started: Project: Microsoft.Windows.CsWin32, Configuration: Debug Any CPU ------
2>C:\Users\Joseph\.nuget\packages\microsoft.build.tasks.git\1.0.0\build\Microsoft.Build.Tasks.Git.targets(24,5): warning : Could not find file 'C:\Users\Joseph\Source\Repos\CsWin32\ext\sdk-api\.git'. The source code won't be available via Source Link.
2>Regenerating C:\Users\Joseph\Source\Repos\CsWin32\obj\Debug\apidocs.yml. This may take a few minutes...
2>Enumerating documents to be parsed...
2>Unhandled exception. System.IO.DirectoryNotFoundException: Could not find a part of the path 'C:\Users\Joseph\Source\Repos\CsWin32\ext\sdk-api\sdk-api-src\content'

@jnm2 jnm2 force-pushed the fixed_char_array_tostring branch from d8e9728 to 66b57b2 Compare June 21, 2021 16:40
@jnm2
Copy link
Contributor Author

jnm2 commented Jun 21, 2021

Nuked it and tried cloning again just now with --recurse-submodules and that appears to be checking out into the right folders.

@jnm2 jnm2 marked this pull request as draft June 21, 2021 16:44
@jnm2
Copy link
Contributor Author

jnm2 commented Jun 21, 2021

@AArnott Somewhere between 4bac0e4 to 4287105 on the main branch, RM_PROCESS_INFO.strAppName and strServiceShortName started being generated as ushort[256] and ushort[64] fields instead of char. This prevents my tests from merging because they rely on generating a type with fixed-length char arrays.

@AArnott
Copy link
Member

AArnott commented Jun 21, 2021

I'm not seeing the ushort you're talking about. I don't see a metadata update between those commits either. I took the latest from main and I see char:

		internal partial struct RM_PROCESS_INFO
		{
			/// <summary>Contains an <a href="https://docs.microsoft.com/windows/desktop/api/restartmanager/ns-restartmanager-rm_unique_process">RM_UNIQUE_PROCESS</a> structure that  uniquely identifies the application by its PID and the time the process began.</summary>
			internal win32.System.RestartManager.RM_UNIQUE_PROCESS Process;
			/// <summary>If the process is a service, this parameter returns the long name for the service. If the process is not a service, this parameter returns the  user-friendly name for the application. If the process is a critical process, and the installer is run  with elevated privileges, this parameter returns the name of the executable file of the critical process. If the process is a critical process, and the installer is run as a service, this parameter returns the long name of the critical process.</summary>
			internal __char_256 strAppName;
			/// <summary>If the process is a service,  this is the short name for the service. This member is not used if the process is not a service.</summary>
			internal __char_64 strServiceShortName;
			/// <summary>Contains an <a href="https://docs.microsoft.com/windows/desktop/api/restartmanager/ne-restartmanager-rm_app_type">RM_APP_TYPE</a> enumeration value that specifies the type of application as <b>RmUnknownApp</b>,  <b>RmMainWindow</b>, <b>RmOtherWindow</b>, <b>RmService</b>, <b>RmExplorer</b> or <b>RmCritical</b>.</summary>
			internal win32.System.RestartManager.RM_APP_TYPE ApplicationType;
			/// <summary>Contains a bit mask that describes the current status of the application. See the <a href="https://docs.microsoft.com/windows/desktop/api/restartmanager/ne-restartmanager-rm_app_status">RM_APP_STATUS</a> enumeration.</summary>
			internal uint AppStatus;
			/// <summary>
			 /// <para>Contains the Terminal Services session ID of the process.  If the terminal session of the process cannot be determined, the value of this member is set to <b>RM_INVALID_SESSION</b> (-1). This member is not used if the process is a service  or a  system critical process.</para>
			/// <para><see href="https://docs.microsoft.com/windows/win32/api//restartmanager/ns-restartmanager-rm_process_info#members">Read more on docs.microsoft.com</see>.</para>
			/// </summary>
			internal uint TSSessionId;
			/// <summary>
			 /// <para><b>TRUE</b> if the application can be restarted by the Restart Manager; otherwise, <b>FALSE</b>. This member is always <b>TRUE</b> if the process is a service. This member is always  <b>FALSE</b> if the process is a critical system process.</para>
			/// <para><see href="https://docs.microsoft.com/windows/win32/api//restartmanager/ns-restartmanager-rm_process_info#members">Read more on docs.microsoft.com</see>.</para>
			/// </summary>
			internal win32.Foundation.BOOL bRestartable;

			internal struct __char_256
			{
				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,_64,_65,_66,_67,_68,_69,_70,_71,_72,_73,_74,_75,_76,_77,_78,_79,_80,_81,_82,_83,_84,_85,_86,_87,_88,_89,_90,_91,_92,_93,_94,_95,_96,_97,_98,_99,_100,_101,_102,_103,_104,_105,_106,_107,_108,_109,_110,_111,_112,_113,_114,_115,_116,_117,_118,_119,_120,_121,_122,_123,_124,_125,_126,_127,_128,_129,_130,_131,_132,_133,_134,_135,_136,_137,_138,_139,_140,_141,_142,_143,_144,_145,_146,_147,_148,_149,_150,_151,_152,_153,_154,_155,_156,_157,_158,_159,_160,_161,_162,_163,_164,_165,_166,_167,_168,_169,_170,_171,_172,_173,_174,_175,_176,_177,_178,_179,_180,_181,_182,_183,_184,_185,_186,_187,_188,_189,_190,_191,_192,_193,_194,_195,_196,_197,_198,_199,_200,_201,_202,_203,_204,_205,_206,_207,_208,_209,_210,_211,_212,_213,_214,_215,_216,_217,_218,_219,_220,_221,_222,_223,_224,_225,_226,_227,_228,_229,_230,_231,_232,_233,_234,_235,_236,_237,_238,_239,_240,_241,_242,_243,_244,_245,_246,_247,_248,_249,_250,_251,_252,_253,_254,_255;

Regarding submodules, you can always get them working on an existing clone with git submodule update --init

@jnm2 jnm2 force-pushed the fixed_char_array_tostring branch 2 times, most recently from 6347c93 to 85efffa Compare June 21, 2021 17:08
@jnm2
Copy link
Contributor Author

jnm2 commented Jun 21, 2021

Oh, I think cloning from my fork caused the submodules to be behind, and nothing fixed that when I checked out the branch I'm working on.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 21, 2021

cd ext\sdk-api; git checkout origin/HEAD seemed to do something, and then I had to rebuild the projects before running tests picked up the change.

@jnm2 jnm2 force-pushed the fixed_char_array_tostring branch from 85efffa to 6579bdc Compare June 21, 2021 17:24
@jnm2
Copy link
Contributor Author

jnm2 commented Aug 31, 2021

Oops, leaving this open so I remember it.

@AArnott
Copy link
Member

AArnott commented Sep 15, 2021

I'm going to take this forward, @jnm2. Thank you for getting it started.

@AArnott AArnott self-assigned this Sep 15, 2021
@AArnott AArnott marked this pull request as ready for review September 15, 2021 16:15
@jnm2
Copy link
Contributor Author

jnm2 commented Sep 15, 2021

@AArnott Good, sorry it's taking me so long to get back to it. If closing the PR is easier, feel free!

@AArnott AArnott merged commit b050877 into microsoft:main Sep 15, 2021
@jnm2 jnm2 deleted the fixed_char_array_tostring branch September 15, 2021 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToString override on fixed char array structs
3 participants