IFACEMETHODIMP and IFACEMETHOD on Windows

[Xiaohan Wang (王消寒)]

Hi,

I'am reviewing some code implementing COM interface and the code looks like this (super simplified example to show the idea).

(1)

class Foo : public IComIsFun {

  IFACEMETHOD(IsFun)(BOOL* fun);

};

IFACEMETHODIMP Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

Personally I am not a fan of the IFACEMETHOD macro where the method name is put in parentheses, which IMHO makes the code harder to read (and will probably confuse clang format). However, it seems it's the recommendation by MS to use IFACEMETHOD to declare a method and then use IFACEMETHODIMP for the implementation.

Then, when I do code search in Chromium code base, it seems the majority of usage looks like this:

(2)

class Foo : public IComIsFun {

  IFACEMETHODIMP IsFun(BOOL* fun) override;

};

HRESULT Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

I feel (2) is much easier to read and more consistent with the rest of Chromium code. But part of me feels we should just follow MS's recommendataion, e.g. the IMP suffix exists for a reason I guess.

FWIW, if you wonder whether we should use these macros at all, there's a previous discussion on this topic and the conclusion is yes we should just use them. Also my example is modified from the example in that thread.

Thoughts?

Xiaohan

[dan...@chromium.org]

+Bruce Dawson 

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YNmOPo37qgJ8SAPgf2oU1kHFjOW0h3tg13a%3DD_O1ZDnog%40mail.gmail.com.

[Daniel Cheng]

I personally think following COM conventions makes sense here. clang-format seems to handle the IFACEMETHOD macro correctly.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaTCrGj5hCxybcVjYdkiMtJ%3D43-OVNF1jG4aJ1E6nkCEtQ%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

Not sure what you meant by "clang-format handle correctly", here's what I get. I feel the IFACEMETHODIMP version is much easier to read.

  IFACEMETHODIMP QueueEvent(MediaEventType type,
                            REFGUID extended_type,
                            HRESULT status,
                            const PROPVARIANT* value) override;

  IFACEMETHOD(QueueEvent)
  (MediaEventType type,
   REFGUID extended_type,
   HRESULT status,
   const PROPVARIANT* value);

[Daniel Cheng]

Well "IMP" means implementation, which a declaration in the header clearly is not :)

As for formatting, I guess my local testing wasn't thorough enough: I didn't test something that wraps multiple lines.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YPC0BVWUqydC2kZA1L2_2n857Szp%2B8-XRUH9yn2PWy_fw%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

Now I've seen multiple cases [1-2] where Microsoft code uses IFACEMETHODIMP for declaration in header files. With the additional benefit of readability and nice formatting, I am now inclined to support (2).

+Bruce Dawson: Thoughts?

[1] https://github.com/microsoft/Windows-classic-samples/blob/master/Samples/Win7Samples/multimedia/mediafoundation/mft_audiodelay/AudioDelayMFT.h  

[2] https://github.com/microsoft/Windows-classic-samples/blob/master/Samples/Win7Samples/multimedia/directshow/baseclasses/winctrl.h  

[Robert Liao]

Where was MS's recommendation? I've primarily seen option (2) running around on Windows (not to say there are some (1) outliers)

The waters are certainly muddy. If you want a story, feel free to continue. Otherwise, I'm inclined to recommend that we go forth with (2).

combaseapi.h provides a story:

 *      These are macros for declaring interfaces.  They exist so that

 *      a single definition of the interface is simulataneously a proper

 *      declaration of the interface structures (C++ abstract classes)

 *      for both C and C++.

 [...]

 *          DECLARE_INTERFACE_(IClassFactory, IUnknown)

 *          {

 *              // *** IUnknown methods ***

 *              STDMETHOD(QueryInterface) (THIS_

 *                                        REFIID riid,

 *                                        LPVOID FAR* ppvObj) PURE;

 

#define STDMETHOD(method)     virtual COM_DECLSPEC_NOTHROW HRESULT STDMETHODCALLTYPE method

#define IFACEMETHOD(method)   __override STDMETHOD(method)

But IFACEMETHODIMP comes along in the more pedestrian winnt.h and ntdef.h.

#define IFACEMETHODIMP    __override STDMETHODIMP

And fortunately __override doesn't have a real meaning as we also get this:

DECLARE_INTERFACE_IID_(ICompositionDrawingSurfaceInterop, IUnknown, "FD04E6E3-FE0C-4C3C-AB19-A07601A576EE")

{

    IFACEMETHOD(BeginDraw)(

        _In_opt_ const RECT * updateRect,

        _In_ REFIID iid,

        _COM_Outptr_ void ** updateObject,

        _Out_ POINT * updateOffset

        ) PURE;

    [...]

}

It's clear that both IFACEMETHOD and IFACEMETHODIMP were intended to be overrides for methods (most of the time). What's the actual difference? Expanded fully:

#define IFACEMETHOD(method)   __override virtual COM_DECLSPEC_NOTHROW HRESULT __export __stdcall method

#define IFACEMETHODIMP        __override                              HRESULT __export __stdcall

For us, the virtual and COM_DECLSPEC_NOTHROW have no effect. We're always overriding, so the virtual isn't necessary and we don't throw, so COM_DECLSPEC_NOTHROW doesn't buy us much.

As a result, they're basically the same for us. With this, code should be fine using IFACEMETHODIMP and HRESULT versions.

[Xiaohan Wang (王消寒)]

Thank you all!

May I conclude that we'd prefer (2) moving forward? Does it make sense to document this decision somewhere, e.g. dos and donts?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/6e07547f-de16-4414-8483-3082515e6e25%40chromium.org.

[Bruce Dawson]

I defer to robliao@'s experience and stories in this area.

[Robert Liao]

(2) is reasonable here. We should probably document this in the Chromium C++ Dos and Don'ts absent a COM Guide for Chromium.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++-dos-and-donts.md

[Xiaohan Wang (王消寒)]

Thank you all for the discussion!

I'll try to draft a CL to add this to "Chromium C++ Dos and Don'ts" if there are no objections.

[Peter Kasting]

On Mon, Feb 10, 2020 at 3:10 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I'll try to draft a CL to add this to "Chromium C++ Dos and Don'ts" if there are no objections.

Ideally that doc should cover items that are of general applicability across a large fraction of the codebase.  I don't think that's a good place for it.

I'm not sure this requires documentation anywhere.  Back in the day I used to advocate for documenting every style decision somewhere, no matter how minor.  But this actually reduces compliance over time since no one can remember the 10,000 decisions, and the important ones get crowded out.  So the question is, is this decision important enough to document that you would sacrifice some other existing rule for it?  If the answer is "definitely not", then I'd just update existing code to comply and let that be "good enough" via the expectation that people will mostly follow existing style.

PK 

[Xiaohan Wang (王消寒)]

Good point and sounds good to me as well!

I can just update existing code and in the future I'll just point to people this discussion thread!

[Xiaohan Wang (王消寒)]

CL prepared here which is a good example of the formatting improvement we discussed.

[K. Moon]

Meta-point: Maybe when someone gets the urge to write guidance about this sort of thing, it could be appropriate for a tip-of-the-week-style article? Those often have not-quite-universal audiences, and the bar is lower for lawyerly perfection.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YMMfzaMMt3v2nTUEseffFgMZtQazkVDjSDOCNmR4JtHLQ%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

I'd assume what we discussed here also apply to STDMETHODIMP and STDMETHOD? There are about 28 files that need to be updated.

[Xiaohan Wang (王消寒)]

media/ CL to update the declaration from STDMETHOD(Foo)() to STDMETHODIMP Foo() is at https://chromium-review.googlesource.com/c/chromium/src/+/2050986.

I noticed that some existing code in Chromium and some Microsoft code use STDMETHODIMP for both declaration and implementation, which seems reasonable to me as well. So I'd suggest we allow both HRESULT and STDMETHODIMP for implementation. Same for  IFACEMETHODIMP:

// Declaration

class Foo : public IComIsFun {

  IFACEMETHODIMP IsFun(BOOL* fun) override;

};

// Implementation. Both are okay.

HRESULT Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

IFACEMETHODIMP Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }  

[Robert Liao]

We should standardize on IFACEMETHODIMP. This makes COM code look the same everywhere.

[Xiaohan Wang (王消寒)]

sgtm!

I used HRESULT earlier in this thread because that's what most of our current code does. I agree having IFACEMETHODIMP on both declaration and implementation makes things more consistent and cleaner.

For the record, here's the recommendation:

// Declaration

class Foo : public IComIsFun {

  IFACEMETHODIMP IsFun(BOOL* fun) override;

};

// Implementation.

IFACEMETHODIMP Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

[Robert Liao]

Sorry, I should have been more clear.

We should standardize on IFACEMETHODIMP (instead of using STDMETHODIMP) for the class declarations but out-of-line implementations should use HRESULT. 

IFACEMETHODIMP brings along extra stuff that isn't necessary helpful.

[Xiaohan Wang (王消寒)]

Ah, sorry I got it wrong.

One question though: Why do we prefer HRESULT in the impl instead of IFACEMETHODIMP?

[Robert Liao]

IFACEMETHODIMP expands to

#define IFACEMETHODIMP        __override                              HRESULT __export __stdcall

These extra annotations don't add much for us for out-of-line annotations as they're already specified on the declaration. As a result, HRESULT should be fine (and is what other COM folks outside of Chromium already do).

[Xiaohan Wang (王消寒)]

I agree HRESULT is totally fine. It's more clear about the actual return type, and is shorter.

On the other hand, IFACEMETHODIMP is also fine to me. It's also used a lot in code outside of Chromium. On top of that, it has the slight benefit that the declaration and implementation look more consistent.

But at the end of the day, I agree just having one recommendation is better, and HRESULT it is!

For the record, here's the recommendation:

// Declaration

class Foo : public IComIsFun {

  IFACEMETHODIMP IsFun(BOOL* fun) override;

};

// Implementation.

HRESULT Foo::IsFun(BOOL* fun) { *fun = false; return S_OK; }

Here's an example CL converting STDMETHOD()/STDMETHODIMP to IFACEMETHODIMP/HRESULT: https://chromium-review.googlesource.com/c/chromium/src/+/2050742  

[Xiaohan Wang (王消寒)]

For those who wonder why we don't need to specify calling convention in the implementation. This is not needed according to https://docs.microsoft.com/en-us/cpp/cpp/stdcall?view=vs-2019

"For non-static class functions, if the function is defined out-of-line, the calling convention modifier does not have to be specified on the out-of-line definition. That is, for class non-static member methods, the calling convention specified during declaration is assumed at the point of definition."