[PATCH] Make an RAII initializer for COM on Windows

[Aaron Ballman]

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?

> +
> +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?

> 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?

> 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? ;-)

> +
> +// 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.

> +
> +namespace llvm {
> +namespace sys {
> +
> +COMInitializer::COMInitializer(COMThreadingMode Threading,
> +                               bool SpeedOverMemory) {
> +  DWORD Coinit = 0;
> +  if (Threading == COMThreadingMode::ApartmentThreaded)
> +    Coinit |= COINIT_APARTMENTTHREADED;
> +  else
> +    Coinit |= COINIT_MULTITHREADED;
> +  if (SpeedOverMemory)
> +    Coinit |= COINIT_SPEED_OVER_MEMORY;
> +  ::CoInitializeEx(nullptr, Coinit);
> +}
> +
> +COMInitializer::~COMInitializer() { ::CoUninitialize(); }
> +}
> +}
>

~Aaron