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

Is there a need to use Array.CreateInstance(typeof(byte)) to create b… #126

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Apr 8, 2024

…yte arrays?

I tried building an application that uses OpenMcdf as NativeAOT (.NET 8.0) and got this warning from the compiler:

16:07:28:585	3>OpenMcdf.CompoundFile.Commit(Boolean): Using member 'System.Array.CreateInstance(Type,Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available.

I guess the code for byte arrays should always exist given they're used elsewhere, but - I'm not sure if there is a need to use Array.CreateInstance rather than just new[] anyway?

@ironfede
Copy link
Owner

ironfede commented Apr 17, 2024

Hi @Numpsy , honestly speaking I can't actually remember the original thinking that pushed me to use this syntax to create arrays... Have you checked Microsoft documentation for some subtle difference? I think that it should work.
Stack Overflow address exactly this point here

@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 18, 2024

I'm not aware of any differences in the date the produce (e.g. whether they differ in how the data is initialized etc).
I can't say offhand if there might have been any difference in some older .NET Version.

@@ -523,7 +523,7 @@ public void Commit(bool releaseMemory)
CheckForLockSector();

sourceStream.Seek(0, SeekOrigin.Begin);
sourceStream.Write((byte[])Array.CreateInstance(typeof(byte), GetSectorSize()), 0, sSize);
sourceStream.Write(new byte[sSize], 0, sSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also changed the array size from GetSectorSize() to sSize (the sSize variable is initialized to GetSectorSize() above, but I didn't think it made sense for the call here to create the array with GetSectorSize size and then call Write with sSize as the size?

@Numpsy Numpsy marked this pull request as ready for review April 18, 2024 19:31
@filipnavara
Copy link

filipnavara commented Jul 7, 2024

@ironfede Can you please merge it? I checked with AOT analyzers turned on that this is the only code flagged as not compatible.

Here's a diff to run the build as net8.0 with the analyzers turned on:

--- a/sources/OpenMcdf/OpenMcdf.csproj
+++ b/sources/OpenMcdf/OpenMcdf.csproj
@@ -1,6 +1,6 @@
 <Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
-    <TargetFrameworks>netstandard2.0;net40</TargetFrameworks>
+    <TargetFrameworks>netstandard2.0;net40;net80</TargetFrameworks>^M
     <Configurations>Debug;Release</Configurations>
     <SignAssembly>true</SignAssembly>
     <!-- Optional: Publish the repository URL in the built .nupkg (in the NuSpec <Repository> element) -->
@@ -18,6 +18,7 @@
     <GenerateAssemblyCopyrightAttribute>false</GenerateAssemblyCopyrightAttribute>
     <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>
     <GenerateAssemblyConfigurationAttribute>false</GenerateAssemblyConfigurationAttribute>
+    <IsAotCompatible Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))">true</IsAotCompatible>^M
   </PropertyGroup>
   <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
     <DebugSymbols>true</DebugSymbols>

@ironfede
Copy link
Owner

ironfede commented Aug 3, 2024

Thank you @Numpsy and @filipnavara for commit and checking compatibility and obviously sorry for not being very active in this times...
Merging now.

@ironfede ironfede merged commit 8276759 into ironfede:master Aug 3, 2024
@Numpsy Numpsy deleted the new_array branch August 5, 2024 17:35
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.

3 participants