Efferent-Health / fo-dicom.Codecs

Cross-platform Dicom native codecs for fo-dicom
Other
64 stars 22 forks source link

Why not use same native dll file names and use single [DllImport] attributes? #43

Closed catester closed 1 year ago

catester commented 1 year ago

Thanks for the library, it works good.

I have a suggestion/question. I noticed you are using boiler plate code everywhere like this:

if (Platform.Current == Platform.Type.win_x64)
    format_message_Windows_x64(ref cinfo, buffer);
else if (Platform.Current == Platform.Type.linux_x64)
    format_message_Linux_x64(ref cinfo, buffer);
else if (Platform.Current == Platform.Type.osx_x64)
    format_message_Osx_x64(ref cinfo, buffer);
else if (Platform.Current == Platform.Type.osx_arm64)
    format_message_Osx_arm64(ref cinfo, buffer);      

[DllImport("Dicom.Native-win-x64.dll", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, EntryPoint = "format_message_8")]
public static extern unsafe void format_message_Windows_x64(ref j_common_ptr cinfo, char[] buffer);

[DllImport("Dicom.Native-linux-x64.so", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, EntryPoint = "format_message_8")]
public static extern unsafe void format_message_Linux_x64(ref j_common_ptr cinfo, char[] buffer);

[DllImport("Dicom.Native-osx-x64.dylib", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, EntryPoint = "format_message_8")]
public static extern unsafe void format_message_Osx_x64(ref j_common_ptr cinfo, char[] buffer);

[DllImport("Dicom.Native-osx-arm64.dylib", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, EntryPoint = "format_message_8")]
public static extern unsafe void format_message_Osx_arm64(ref j_common_ptr cinfo, char[] buffer

As far as I know when you use a common name for the native DLL, then it is found via adding the native file extension automatically (e.g. .dll for Windows, .so for Linux)

[DllImport("Dicom.Native")]

So if you use these names for the native DLLs Dicom.Native.so Dicom.Native.dylib Dicom.Native.dll

Then the above DllImport will automatically work for all. Reference

If you are worried about users confusing the native files, first they are already in specific runtime subfolder, and also you can always embed platform description in PE metadata, e.g. description can be "Dicom.Native (win-x64)"

So you wouldn't need to have 4 different imports for a single function, as long as all functions have same name in all native files (I noticed it is so) and you wouldn't need to have if/switch statements for all 4 platforms? This could make maintenance and adding new platforms easier?

 format_message(ref cinfo, buffer);

[DllImport("Dicom.Native", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, EntryPoint = "format_message_8")]
public static extern unsafe void format_message(ref j_common_ptr cinfo, char[] buffer);
jaime-olivares commented 1 year ago

We are already evaluating this suggestion

cbeltran1306 commented 1 year ago

Hi @catester, this improvement has been included into the new published nuget 5.10.6. Thank you for your suggestion!