[PATCH] Make an RAII initializer for COM on Windows

[Aaron Ballman]

On Mon, Apr 27, 2015 at 12:47 PM, Zachary Turner <zturner at google.com> wrote:
>
>
> On Mon, Apr 27, 2015 at 5:52 AM Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Fri, Apr 24, 2015 at 5:39 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> > Hi aaron.ballman, compnerd,
>> >
>> > Patch should be fairly self-explanatory.  Implementation is empty on
>> > non-Windows platforms, and on Windows platforms simply calls CoInitializeEx
>> > and CoUninitialize.
>> >
>> > http://reviews.llvm.org/D9267
>> >
>> > Files:
>> >   include/llvm/Support/COM.h
>> >   lib/Support/CMakeLists.txt
>> >   lib/Support/COM.cpp
>> >   lib/Support/Unix/COM.inc
>> >   lib/Support/Windows/COM.inc
>> >
>> > EMAIL PREFERENCES
>> >   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>> In addition to David's suggestion, some minor nits and questions below:
>>
>> > Index: include/llvm/Support/COM.h
>> > ===================================================================
>> > --- /dev/null
>> > +++ include/llvm/Support/COM.h
>> > @@ -0,0 +1,35 @@
>> > +//===- llvm/Support/COM.h ---------------------------------------*- C++
>> > -*-===//
>> > +//
>> > +//                     The LLVM Compiler Infrastructure
>> > +//
>> > +// This file is distributed under the University of Illinois Open
>> > Source
>> > +// License. See LICENSE.TXT for details.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +/// \file
>> > +///
>> > +/// Provides a library for accessing COM functionality of the Host OS.
>> > +///
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +
>> > +#ifndef LLVM_SUPPORT_COM_H
>> > +#define LLVM_SUPPORT_COM_H
>> > +
>> > +namespace llvm {
>> > +namespace sys {
>> > +
>> > +enum class COMThreadingMode { ApartmentThreaded, FreeThreaded };
>>
>> While I know that these are the COM terms, they're pretty
>> unintelligible for folks who aren't used to COM. While not 100%
>> accurate, do you think SingleThreaded and MultiThreaded would be more
>> clear terms?
>
> I'm not sure which is better.  On the one hand someone who doesn't
> understand how COM works shouldn't be using this code in the first place,
> but on the other hand I agree that it's hard to understand even if you do
> know COM.  I don't feel strongly either way, but in any case I've made the
> chance locally to Single and Multi.

I wasn't worried about new code so much as casual readers from other
platforms seeing "apartment" and being confused.

>
>>
>>
>> > +
>> > +class COMInitializer {
>> > +public:
>> > +  COMInitializer(COMThreadingMode Threading, bool SpeedOverMemory =
>> > false);
>>
>> Explicit constructor (though not overly important)?
>>
>> > +  ~COMInitializer();
>> > +
>> > +private:
>> > +  COMInitializer(const COMInitializer &) = delete;
>> > +  void operator=(const COMInitializer &) = delete;
>> > +};
>> > +}
>> > +}
>> > +
>> > +#endif
>> > Index: lib/Support/CMakeLists.txt
>> > ===================================================================
>> > --- lib/Support/CMakeLists.txt
>> > +++ lib/Support/CMakeLists.txt
>> > @@ -37,6 +37,7 @@
>> >    BlockFrequency.cpp
>> >    BranchProbability.cpp
>> >    circular_raw_ostream.cpp
>> > +  COM.cpp
>> >    CommandLine.cpp
>> >    Compression.cpp
>> >    ConvertUTF.c
>>
>> Do we need to do any special magic to ensure Ole32 is linked in, or
>> have we always linked against that?
>
> It's always linked in.  ole32 is one of the defaultlibs, and we don't
> specify /NODEFAULTLIB
>
>>
>>
>> > Index: lib/Support/COM.cpp
>> > ===================================================================
>> > --- /dev/null
>> > +++ lib/Support/COM.cpp
>> > @@ -0,0 +1,24 @@
>> > +//===-- COM.cpp - Implement COM utility classes -----------------*- C++
>> > -*-===//
>> > +//
>> > +//                     The LLVM Compiler Infrastructure
>> > +//
>> > +// This file is distributed under the University of Illinois Open
>> > Source
>> > +// License. See LICENSE.TXT for details.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +//
>> > +//  This file implements utility classes related to COM.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +
>> > +#include "llvm/Support/COM.h"
>> > +
>> > +#include "llvm/Config/config.h"
>> > +
>> > +// Include the platform-specific parts of this class.
>> > +#ifdef LLVM_ON_UNIX
>> > +#include "Unix/COM.inc"
>> > +#endif
>> > +#ifdef LLVM_ON_WIN32
>> > +#include "Windows/COM.inc"
>> > +#endif
>>
>> Use #elseif?
>
> Done.
>
>>
>>
>> > Index: lib/Support/Unix/COM.inc
>> > ===================================================================
>> > --- /dev/null
>> > +++ lib/Support/Unix/COM.inc
>> > @@ -0,0 +1,27 @@
>> > +//===- llvm/Support/Unix/COM.inc - Unix COM Implementation -----*- C++
>> > -*-===//
>> > +//
>> > +//                     The LLVM Compiler Infrastructure
>> > +//
>> > +// This file is distributed under the University of Illinois Open
>> > Source
>> > +// License. See LICENSE.TXT for details.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +//
>> > +// This file implements the Unix portion of COM support.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +//=== WARNING: Implementation here must contain only generic UNIX code
>> > that
>> > +//===          is guaranteed to work on *all* UNIX variants.
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +
>> > +namespace llvm {
>> > +namespace sys {
>> > +
>> > +COMInitializer::COMInitializer(COMThreadingMode Threading,
>> > +                               bool SpeedOverMemory) {}
>> > +
>> > +COMInitializer::~COMInitializer() {}
>> > +}
>> > +}
>> > Index: lib/Support/Windows/COM.inc
>> > ===================================================================
>> > --- /dev/null
>> > +++ lib/Support/Windows/COM.inc
>> > @@ -0,0 +1,41 @@
>> > +//===- llvm/Support/Windows/COM.inc - Windows COM Implementation *- C++
>> > -*-===//
>> > +//
>> > +//                     The LLVM Compiler Infrastructure
>> > +//
>> > +// This file is distributed under the University of Illinois Open
>> > Source
>> > +// License. See LICENSE.TXT for details.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +//
>> > +// This file implements the Windows portion of COM support.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +//=== WARNING: Implementation here must contain only generic UNIX code
>> > that
>> > +//===          is guaranteed to work on *all* UNIX variants.
>> >
>> > +//===----------------------------------------------------------------------===//
>>
>> Can you fix the comments regarding UNIX vs Windows? ;-)
>
> Done.
>
>>
>>
>> > +
>> > +// We need to #include <windows.h> directly, and not WindowsSupport.h,
>> > because
>> > +// WindowsSupport.h #defines WIN32_LEAN_AND_MEAN, which will prevent
>> > COM headers
>> > +// from getting included.
>> > +#include <windows.h>
>>
>> Do we need *all* of Windows.h though? I would guess that all we would
>> need is Objbase.h here, and not Windows.h at all.
>
> Didn't think this would work originally because I was using DWORD, but I
> tried it and it does work, so this is done.

Great; the last bit is the name change David suggested, and with that,
it'll LGTM.

~Aaron