<div dir="ltr"><br><br><div class="gmail_quote">On Mon, Apr 27, 2015 at 5:52 AM Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Apr 24, 2015 at 5:39 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> Hi aaron.ballman, compnerd,<br>
><br>
> Patch should be fairly self-explanatory. Implementation is empty on non-Windows platforms, and on Windows platforms simply calls CoInitializeEx and CoUninitialize.<br>
><br>
> <a href="http://reviews.llvm.org/D9267" target="_blank">http://reviews.llvm.org/D9267</a><br>
><br>
> Files:<br>
> include/llvm/Support/COM.h<br>
> lib/Support/CMakeLists.txt<br>
> lib/Support/COM.cpp<br>
> lib/Support/Unix/COM.inc<br>
> lib/Support/Windows/COM.inc<br>
><br>
> EMAIL PREFERENCES<br>
> <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
In addition to David's suggestion, some minor nits and questions below:<br>
<br>
> Index: include/llvm/Support/COM.h<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ include/llvm/Support/COM.h<br>
> @@ -0,0 +1,35 @@<br>
> +//===- llvm/Support/COM.h ---------------------------------------*- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +/// \file<br>
> +///<br>
> +/// Provides a library for accessing COM functionality of the Host OS.<br>
> +///<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#ifndef LLVM_SUPPORT_COM_H<br>
> +#define LLVM_SUPPORT_COM_H<br>
> +<br>
> +namespace llvm {<br>
> +namespace sys {<br>
> +<br>
> +enum class COMThreadingMode { ApartmentThreaded, FreeThreaded };<br>
<br>
While I know that these are the COM terms, they're pretty<br>
unintelligible for folks who aren't used to COM. While not 100%<br>
accurate, do you think SingleThreaded and MultiThreaded would be more<br>
clear terms?<br></blockquote><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> +class COMInitializer {<br>
> +public:<br>
> + COMInitializer(COMThreadingMode Threading, bool SpeedOverMemory = false);<br>
<br>
Explicit constructor (though not overly important)?<br>
<br>
> + ~COMInitializer();<br>
> +<br>
> +private:<br>
> + COMInitializer(const COMInitializer &) = delete;<br>
> + void operator=(const COMInitializer &) = delete;<br>
> +};<br>
> +}<br>
> +}<br>
> +<br>
> +#endif<br>
> Index: lib/Support/CMakeLists.txt<br>
> ===================================================================<br>
> --- lib/Support/CMakeLists.txt<br>
> +++ lib/Support/CMakeLists.txt<br>
> @@ -37,6 +37,7 @@<br>
> BlockFrequency.cpp<br>
> BranchProbability.cpp<br>
> circular_raw_ostream.cpp<br>
> + COM.cpp<br>
> CommandLine.cpp<br>
> Compression.cpp<br>
> ConvertUTF.c<br>
<br>
Do we need to do any special magic to ensure Ole32 is linked in, or<br>
have we always linked against that?<br></blockquote><div>It's always linked in. ole32 is one of the defaultlibs, and we don't specify /NODEFAULTLIB</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Index: lib/Support/COM.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ lib/Support/COM.cpp<br>
> @@ -0,0 +1,24 @@<br>
> +//===-- COM.cpp - Implement COM utility classes -----------------*- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// This file implements utility classes related to COM.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#include "llvm/Support/COM.h"<br>
> +<br>
> +#include "llvm/Config/config.h"<br>
> +<br>
> +// Include the platform-specific parts of this class.<br>
> +#ifdef LLVM_ON_UNIX<br>
> +#include "Unix/COM.inc"<br>
> +#endif<br>
> +#ifdef LLVM_ON_WIN32<br>
> +#include "Windows/COM.inc"<br>
> +#endif<br>
<br>
Use #elseif?<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Index: lib/Support/Unix/COM.inc<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ lib/Support/Unix/COM.inc<br>
> @@ -0,0 +1,27 @@<br>
> +//===- llvm/Support/Unix/COM.inc - Unix COM Implementation -----*- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// This file implements the Unix portion of COM support.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +//===----------------------------------------------------------------------===//<br>
> +//=== WARNING: Implementation here must contain only generic UNIX code that<br>
> +//=== is guaranteed to work on *all* UNIX variants.<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +namespace llvm {<br>
> +namespace sys {<br>
> +<br>
> +COMInitializer::COMInitializer(COMThreadingMode Threading,<br>
> + bool SpeedOverMemory) {}<br>
> +<br>
> +COMInitializer::~COMInitializer() {}<br>
> +}<br>
> +}<br>
> Index: lib/Support/Windows/COM.inc<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ lib/Support/Windows/COM.inc<br>
> @@ -0,0 +1,41 @@<br>
> +//===- llvm/Support/Windows/COM.inc - Windows COM Implementation *- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// This file implements the Windows portion of COM support.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +//===----------------------------------------------------------------------===//<br>
> +//=== WARNING: Implementation here must contain only generic UNIX code that<br>
> +//=== is guaranteed to work on *all* UNIX variants.<br>
> +//===----------------------------------------------------------------------===//<br>
<br>
Can you fix the comments regarding UNIX vs Windows? ;-)<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> +// We need to #include <windows.h> directly, and not WindowsSupport.h, because<br>
> +// WindowsSupport.h #defines WIN32_LEAN_AND_MEAN, which will prevent COM headers<br>
> +// from getting included.<br>
> +#include <windows.h><br>
<br>
Do we need *all* of Windows.h though? I would guess that all we would<br>
need is Objbase.h here, and not Windows.h at all.<br></blockquote><div>Didn't think this would work originally because I was using DWORD, but I tried it and it does work, so this is done.</div></div></div>