[Mono-dev] [PATCH] New managed code for unmanaged (Win32) resource handling (SRE, PEAPI, MCS)

[Marek Safar]

Hello Kornél,
> This patch adds a fully managed and object oriented unmanaged (Win32)
> resource handling internal infractructure to the class library. The most
> important change is that it implements the /resource command line 
> option of
> ilasm in PEAPI.
>
> I revorked DefineUnmanagedResource and DefineVersionInfoResource 
> methods to
> be fully MS.NET compatible. Including resource blob support.
>
> In addition I replaced the previous unamaged implementation with the same
> unmanaged code that is used in PEAPI. This has the following advantages:
> - Named resource identifiers are supported
> - Very large .res files are supported because resources are not 
> retained in
> memory
Well, I would prefer to use the memory solution.
1. It's faster
2. It's easier to implement, no problems with file removing, creating, 
locking, etc.
3. I don't believe that anyone will link more than 10 MB resource file.

> - Has exactly the same behavior as MS cvtres.exe that is used by the MS
> runtime and csc.exe as well including the some tests and exceptions 
> thrown
> by the wrapper code of MS runtime.
>
> I removed the internal DefineIconResource method and moved it's
> functionality to mcs compiler that now uses DefineUnmanagedResource.
>
> DefineVersionInfoResource and mcs no generate two differend and MS.NET
> compatible version info resource files.
I reviewed just mcs and corlib part of the patch and here are my comments.

+                    } catch (Exception ex) {
+                        Report.Error (1567, string.Format ("Error 
generating Win32 resource: {0}", ex.Message));
+                        return false;
+                    }

1. You don't need to use string.Format as Report.Error accepts variable 
arguments.
2.  Please provide error tests for as many new error codes as possible.

-                MethodInfo define_icon = typeof 
(AssemblyBuilder).GetMethod ("DefineIconResource", 
BindingFlags.Instance|BindingFlags.Public|BindingFlags.NonPublic);
-                if (define_icon == null) {
-                    Report.RuntimeMissingSupport (Location.Null, 
"resource embeding");

I prefer this method of code sharing instead of copying all the sources 
to every file. Please use reflection to get instance of your internal 
classes. I know you are using several fields, so if you really need them 
then a solution can be to introduce interface e.g. IResource with all 
required properties and methods and distribute only this interface file 
across. Of course, you will need some factory method to create this 
interface but that is the trivial part.


+        private static void CreateDefaultResource (string fileName)
+        {

Try to move this method to CodeGen.Assembly class.

+        public static string CreateStringTableKey(ushort languageId, 
ushort codePage, bool upperCase)
+        {
+            return ((languageId << 16) | codePage).ToString(upperCase ? 
"X8" : "x8", CultureInfo.InvariantCulture);
+        }

Please use constants instead of X8, x8

+            base.WriteResources(resourceFile);
+
+            if (this.iconFileName != null)
+            {
+                stream = new FileStream(this.iconFileName, 
FileMode.Open, FileAccess.Read, FileShare.Read);

Please use style

if (this.iconFileName == null)
    return;



Regards,
Marek