nit: I think rewriting this as below is equally performant and certainly cleaner

nit:

nit: you can delete the else here. Might even be worth moving the entire if to the top of this block since its quite short comparted to the other one

nit:

nit: it looks like these can be a single if statement

For one, you'll need to wrap all the interfaces that the compiler object implements so you don't lose the object wrapping for any of them.

But also, IDxcCompiler and IDxcCompiler3 should be incompatible to derive from and implement in the same class due to the nature of COM not supporting separate overloaded methods of the same name. This requires a separate object to implement the other interfaces.

So to be clear, are you suggesting one parent class, ExternalValidationHelper, that implements 4 interfaces and has 4 wrapper member objects, that each implement one interface: IDxcCompiler, IDxcCompiler2, IDxcCompiler3, and IDxcValidator? Your comment above, "This code should all be inside the compiler object wrapper.", makes me think that this class should also implement the IDxcValidator interface, is that correct?

But also, IDxcCompiler and IDxcCompiler3 should be incompatible to derive from and implement in the same class due to the nature of COM not supporting separate overloaded methods of the same name.

It's lucky that IDxcCompiler and IDxcCompiler3 don't actually have any identical methods in them so that this isn't a problem.

IDxcCompiler::Compile takes 10 args, while IDxcCompiler3::Compile takes 7
Preprocess only exists on IDxcCompiler
IDxcCompiler::Disassemble takes 2 args while IDxcCompiler3::Disassemble take 3.

If you only try and implement one of these when you derive from both interfaces you'll get a warning about hiding the base class version.

Interesting. I thought it was not allowed, hence the hoops we jumped through multiple times for multiple implementations of the compiler object supporting both interfaces to avoid deriving from both on the same implementation class.

In any case the first point stands, that there are more interfaces to support than these two.

You'll want to derive from each of these:

• IDxcCompiler2
• IDxcCompiler3
• IDxcLangExtensions3
• IDxcContainerEvent
• IDxcVersionInfo3
• IDxcVersionInfo2 (or IDxcVersionInfo if SUPPORT_QUERY_GIT_COMMIT_INFO not defined)

And no need to derive from IDxcCompiler when deriving from IDxcCompiler2, since IDxcCompiler2 already derives from IDxcCompiler.

The implementation in this PR will cause failures if any of the tests attempt to use these interfaces. I don't think we need to wrap additional ones for the sake of it.

Like Damyan said, I think I'll worry about deriving from each of those in a future PR.

I suggest returning E_POINTER as I think that's what this HRESULT is intended to reflect?
That is, we expect that the caller gives us a valid IDxcOperationResult*.

The UseResult name seems misleading to me? It really looks like it's just a glorified QueryInterface call?

I think it would be cleaner to

1. Remove the UseResult lambda
2. Add a check to return E_INVALID_ARG if CompileResult is an invalid pointer at the start of doValidation.
3. Update line 59 to 'return CompileResult->QueryInterface(Riid, ValResuilt);
4. Same update to line 67.
5. Same update for line 83.
6. Same update on line 87.

I think the exception expected here would be due to an allocation failure.
So would ERROR_OUTOFMEMORY be a more appropriate HRESULT?

nit: I like to try and avoid E_FAIL because its pretty generic.
There is an 'ERROR_VERSION_PARSE_ERROR' you could use here.

Gonna stick to E_FAIL since there is no definition for that Error code in non-windows.
Same applies to ERROR_OUTOFMEMORY

I think we generally do all publics first and then all privates second?
Should we move the member variables to the private section?

nit: public first for consistency with the rest of this file

Actually the reason this broke that consistency is because there's a special rule in linux / macos builds on initialization order, and the Compiler object can't be initialized after the m_pAlloc field, which was originally taken care of in the previous private block. So I will leave this as is.

there's a special rule in linux / macos builds on initialization order

There's not a special rule, it's just that MSVC doesn't warn if you mismatch the order that things appear in the constructor's initialization list with the order that they are actually initialized (ie the order they appear in the class itself).

If you wanted to put the private block at the end then you'd just need to update the constructor to list m_pMalloc first (since it'd be listed first in the class as the field is added by the DXC_MICROCOM_TM_REF_FIELDS macro).

Or you could have your private block at the end look like this and not need to modify the constructor:

private:
  CComPtr<IDxcValidator> Validator;
  IID CompilerIID;
  CComPtr<IUnknown> Compiler;
  DXC_MICROCOM_TM_REF_FIELDS()

nit: comment doesn't seem accurate. We're returning the HRESULT of QI. That doesn't seem like a compile result?

This language is a bit overloaded, I meant return through an out-parameter, by placing the result into ValResult.
I can rewrite this.

The main thing that threw me off is that you wouldn't generally expect a QI call to do anything other than get you a pointer to the requested interface. The fact that the private implementation of QI is doing more than that is a little weird.

I see uses of DXC_FAILED and FAILED. Hadn't seen DXC_FAILED before but it looks like they do the same thing.
We should just use one.