Thomas Faber

There's also of course the option of using a namespace.

There's also of course the option of using a namespace.

This seemed better as a switch. There are more RTL languages than just Hebrew.

This seemed better as a switch. There are more RTL languages than just Hebrew.

Do we do this elsewhere? It's kind of weird to add header files (the PCH aside) to this list.

Do we do this elsewhere? It's kind of weird to add header files (the PCH aside) to this list.

This and IsAvailableEnum should either be both macros or both functions

This and IsAvailableEnum should either be both macros or both functions

Spacing looks off

Spacing looks off

Why is this an improvement over having this stuff in rapps.h?

Why is this an improvement over having this stuff in rapps.h?

Was there consensus on the ellipses? I see other strings that still use "..."

Was there consensus on the ellipses? I see other strings that still use "..."

This doesn't make sense grammatically. Presumably other languages will have the same problem. Not sure what the right approach is... revert to English? Check with individual translators?

This doesn't make sense grammatically. Presumably other languages will have the same problem.
Not sure what the right approach is... revert to English? Check with individual translators?

missing newline. Double-check for this kind of problem in all files please.

missing newline. Double-check for this kind of problem in all files please.

Not sure what the point of all these style changes is

Not sure what the point of all these style changes is

The constructor must set this to null, and this should check for null before calling FreeLibrary

The constructor must set this to null, and this should check for null before calling FreeLibrary

m_

m_

This might be more readable with if-else if instead of double ?:

This might be more readable with if-else if instead of double ?:

'inline' here does nothing.

'inline' here does nothing.

const on a return type doesn't make sense. Doesn't this produce a warning? Consider const on the function though.

const on a return type doesn't make sense. Doesn't this produce a warning?
Consider const on the function though.

This feels really odd in C++, I'd probably prefer a member initializer list in the constructor

This feels really odd in C++, I'd probably prefer a member initializer list in the constructor

I'd prefer if this either: *was a POD type (stays a struct, does not have a leading C and keeps the member names like this), or *became a class and used m_ prefix for members

I'd prefer if this either:

  • was a POD type (stays a struct, does not have a leading C and keeps the member names like this), or
  • became a class and used m_ prefix for members
This enum uses a complete different style both for its name and its members than the one above. Deciding on one style would be nice.

This enum uses a complete different style both for its name and its members than the one above. Deciding on one style would be nice.

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