google / UIforETW

User interface for recording and managing ETW traces
https://randomascii.wordpress.com/2015/04/14/uiforetw-windows-performance-made-easier/
Apache License 2.0
1.57k stars 201 forks source link

etwprof.h: Adjust guard to only enable under MSVC, and allow a disable define #44

Closed rpavlik closed 9 years ago

rpavlik commented 9 years ago

The previous code conflates building for Windows with building with MSVC, while we'd like consuming code to be able to build with other compilers on Windows too.

randomascii commented 9 years ago

Why do you only want ETW enabled under MSVC? ETW is available under Windows and the compiler used should be irrelevant. Adding a #define to allow explicit disabling could be helpful.

Note that before accepting pull requests I need you to sign the CLA -- see the instructions at https://github.com/google/UIforETW/blob/master/CONTRIBUTING

rpavlik commented 9 years ago

Err... so I guess that I:

  1. glossed over the fact that compiler != system sdk given that now I can use clang in visual studio
  2. assumed that mingw didn't have the required headers, probably because I...
  3. saw the SAL annotations and figured those wouldn't work without MSVC (I've typically done project-specific wrapping of SAL annotations I want to use that define them typically to nothing on non-msvc, though I suppose I haven't actually tested building with them left in on a modern mingw or clang-cl

Oops.

I think the only issue there might be the SAL annotations, because if nothing else it's a C API so I should be able to use an MSVC-built library with mingw even if the headers used by the implementation are missing there.

Thought I signed a CLA back in the day (for IWYU) but I see new web app since then, and new employer, so will go through that again.

randomascii commented 9 years ago

I looked up your github username and email address and saw no sign of a CLA. Sorry about the requirement, but it is a requirement.

SAL annotations are easy to support with other compilers because they can be preprocessed to nothing - at least I think that is how it works. Either that or they can be ignored by the compiler. That's not to say that they are supported, but I wouldn't be surprised if clang-on-Windows can handle having them there.

rpavlik commented 9 years ago

OK, there should now be a CLA through my employer (firstname at sensics.com).

I guess I'll have to test the SAL thing some more.

ariccio commented 9 years ago

SAL annotations are easy to support with other compilers because they can be preprocessed to nothing

Yup. (the way that it's usually done is to) Create a file called NoSAL.h (or whatever), and just #define all the macros as nothing.

I assume that Clang-on-windows does something similar, else it'd have some trouble with Windows.h.

rpavlik commented 9 years ago

OK, so with a dummy SAL (https://github.com/OSVR/OSVR-Core/tree/master/vendor/dummy-sal), and now that it's all extern "C" I can build against it (though not build it - can't compile the instrumentation manifest) with MinGW, closing this out.