Thomas Faber

I believe that means you should do the same thing that this line does with IOleWindow also with IDockingWindow

I believe that means you should do the same thing that this line does with IOleWindow also with IDockingWindow

The way InitializeSFB is currently implemented, it does take ownership. I think that's unusual for a COM interface though. InitializeSFB should probably do an ILClone, and its caller should free th...

The way InitializeSFB is currently implemented, it does take ownership. I think that's unusual for a COM interface though. InitializeSFB should probably do an ILClone, and its caller should free the original pidl.

Right, this was before I read the implementation. If it stays the way it's currently implemented, a common base class is not needed.

Right, this was before I read the implementation. If it stays the way it's currently implemented, a common base class is not needed.

And really it's == 0, it can't be less since it's unsigned

And really it's == 0, it can't be less since it's unsigned

As opposed to half the attribute length?

As opposed to half the attribute length?

There's actually a downside to doing it now: it breaks special pool's ability to detect uses after free (because the allocations aren't actually freed when you return them to the lookaside list)

There's actually a downside to doing it now: it breaks special pool's ability to detect uses after free (because the allocations aren't actually freed when you return them to the lookaside list)

It's hard to tell where "here" is because your patch isn't anchored :\

It's hard to tell where "here" is because your patch isn't anchored :\

Unnecessary change

Unnecessary change

Isn't that simply a performance optimization?

Isn't that simply a performance optimization?

This is IUnknown_QueryInterface (that's the C macro name for this call) https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/wink.gif

This is IUnknown_QueryInterface (that's the C macro name for this call)

Or just if (KeyValueInformationClass > KeyValuePartialInformationAlign64)

Or just if (KeyValueInformationClass > KeyValuePartialInformationAlign64)

Not clear on why this needs to be a parameter

Not clear on why this needs to be a parameter

Looking at the current code, nothing that would need to be freed can possibly have been set at this point.

Looking at the current code, nothing that would need to be freed can possibly have been set at this point.

You zeroed it above so don't technically need this

You zeroed it above so don't technically need this

Why not just put the function up here?

Why not just put the function up here?

I highly doubt it. If there is a warning, someone show it to me. Then I'll tell you why the cast is still the wrong solution. If there is no warning, there's no need for a cast https://code.reactos...

I highly doubt it.
If there is a warning, someone show it to me. Then I'll tell you why the cast is still the wrong solution.
If there is no warning, there's no need for a cast

Yeah, that ^

Yeah, that ^

I highly doubt it.

I highly doubt it.

It seems to be what's generally done in this file, so this is making things more consistent. That said, I would suggest not making unnecessary style changes at all.

It seems to be what's generally done in this file, so this is making things more consistent. That said, I would suggest not making unnecessary style changes at all.

Actually you don't need to initialize this at all, that's _GetInstalledVersion's job

Actually you don't need to initialize this at all, that's _GetInstalledVersion's job

So, the whole concept of ParseVersion relies on the idea that you will be able to parse all possible version strings – which seems flawed. Why not do a string comparison where numbers are treated s...

So, the whole concept of ParseVersion relies on the idea that you will be able to parse all possible version strings – which seems flawed.
Why not do a string comparison where numbers are treated specially, like sort --numeric-sort. CompareString[Ex] has a SORT_DIGITSASNUMBERS flag that probably does exactly that. It appears to be Win7+ but you might be able to find an alternative function, or implement a mechanism like this yourself.

This is a little too magical for my taste. If you want to make this struct non-POD (which is what you're doing here), please add an explicit constructor.

This is a little too magical for my taste. If you want to make this struct non-POD (which is what you're doing here), please add an explicit constructor.

Actually, your destination buffer size here is sizeof(Version->szVersion)

Actually, your destination buffer size here is sizeof(Version->szVersion)

You mean sizeof(szVersion[0]), otherwise you're taking the size of the pointer. Or you could use the Cch version of the function and remove the multiplication

You mean sizeof(szVersion[0]), otherwise you're taking the size of the pointer. Or you could use the Cch version of the function and remove the multiplication

This doesn't work as a template, you're always calling the -W version of the function, because UNICODE is defined.

This doesn't work as a template, you're always calling the -W version of the function, because UNICODE is defined.

That's not useful, UINT and ULONG have the same value range

That's not useful, UINT and ULONG have the same value range

Only call this in case of success, i.e. inside the if

Only call this in case of success, i.e. inside the if

RegQueryValueEx uses byte counts, not character counts. Changing the variable's name to cbSize could make this clearer.

RegQueryValueEx uses byte counts, not character counts. Changing the variable's name to cbSize could make this clearer.

You'll want to do this only in case of success, otherwise you didn't get a valid hKey

You'll want to do this only in case of success, otherwise you didn't get a valid hKey

Superfluous newline

Superfluous newline