[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

Thu Feb 11 12:22:02 PST 2021

stella.stamenova added inline comments.

================
Comment at: lldb/tools/lldb-vscode/FifoFiles.cpp:9
+
+#if !defined(WIN32)
+#include <sys/stat.h>
----------------
mstorsjo wrote:
> stella.stamenova wrote:
> > Also, this is no good. It works if you are targeting windows on windows, but not if you are targeting something else and building on windows. There are a few different ways this is done in LLVM/clang and they are generally not based on defined(WIN32). Here are a couple of examples:
> > 
> > From llvm\lib\Support\Path.cpp:
> > 
> > ```
> > #if !defined(_MSC_VER) && !defined(__MINGW32__)
> > ```
> > 
> > From clang\lib\Driver\Driver.cpp:
> > 
> > ```
> > #if LLVM_ON_UNIX
> > ```
> > 
> > From llvm\lib\Support\ErrorHandling.cpp:
> > 
> > ```
> > #if defined(HAVE_UNISTD_H)
> > ```
> > 
> > I suggest browsing through the code and finding the most appropriate way to manage your includes. In the mean time, this is breaking our internal builds that run on Windows but do not target Windows.
> @stella.stamenova `defined(WIN32)` shouldn't be used (as that isn't predefined by the compiler), but `defined(_WIN32)` should be just fine (and is used a lot within LLDB already, 176 occurrances). If cross compiling on windows, targeting another os, neither of them should be defined, so it should be just fine, no?
> 
> Or is this a case of cygwin, where the lines are blurred even more?
> 
> Does the current main branch work fine for you?
`defined(_WIN32)` is fine for us, since we build on Windows (and not cygwin). I have not tried the lldb-vscode build on cygwin, so that may or may not work with the current setup.

In general, there are too many ways in LLVM to achieve the same excludes/includes right now and the lack of unity is only creating issues and confusion.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93951/new/

https://reviews.llvm.org/D93951