search

dotnet / pinvoke

A library containing all P/Invoke code so you don't have to import it every time. Maintained and updated to support the latest Windows OS.
MIT License
2.12k stars 222 forks source link

Add CfgMgr32 API #473

Closed qmfrederik closed 4 years ago

qmfrederik commented 4 years ago
AArnott commented 4 years ago

@qmfrederik Can you review the commit I just pushed and let me know if it still looks good to you? Manual testing shows the callback code works.

AArnott commented 4 years ago

BTW now that it is as easy as new string(eventData->InstanceId), I wonder if we even really need the helper methods you added. In #480 you seem to be content with the caller using the string constructor directly.

qmfrederik commented 4 years ago

@AArnott Thanks for bearing with me and all the valuable feedback!

In #480 new string(sbyte*) is used because a) the API guarantees the string is \0-terminated and b) the API does not provide you with the total length of the string.

In this case, you are able to determine the full length of the string (by inspecting the size of the struct). If you do new string(char*), you're ignoring that extra information at the risk of a buffer overflow.

So the question is whether you want to be explicit about the string length when reading from unmanaged memory or not. I don't have a strong opinion on the topic - either way is fine with me.

AArnott commented 4 years ago

So the question is whether you want to be explicit about the string length when reading from unmanaged memory or not. I don't have a strong opinion on the topic - either way is fine with me.

Does the documentation indicate there will always be a null terminator? If so, I don't mind depending on it. Your version trimmed the prescribed length by one character to remove the null terminator so it appears that it's a guaranteed thing, or you didn't mind trimming the last character either way.

So assuming there's always a null, shall we remove the helper methods?