[libc-commits] [PATCH] D134676: [libc][windows] fix small build issues.

Mon Sep 26 16:35:55 PDT 2022

michaelrj added inline comments.

================
Comment at: libc/src/CMakeLists.txt:12

-if(${LIBC_TARGET_OS} STREQUAL "linux")
+if((${LIBC_TARGET_OS} STREQUAL "linux") OR (${LIBC_TARGET_OS} STREQUAL "windows"))
   add_subdirectory(dirent)
----------------
sivachandra wrote:
> I think this is a case of inappropriate design somewhere - adding these directories for Windows incorrectly implies that they are available for Windows as well.
as far as I can tell, the issue is coming from `remove_test` which is in `test/src/stdio`. It includes `libc.src.unistd.access` which doesn't exist unless the `unistd` folder is included. Putting that inside an "if target is linux" condition also works, but I feel like that is prone to mistakes for future targets that also need linux only pieces.

================
Comment at: libc/src/__support/FPUtil/x86_64/FEnvImpl.h:57
   static constexpr uint16_t DIV_BY_ZERO = 0x4;
-  static constexpr uint16_t OVERFLOW = 0x8;
-  static constexpr uint16_t UNDERFLOW = 0x10;
----------------
sivachandra wrote:
> lntue wrote:
> > Can you also change other exception flags to the same format too?
> Where is the macro named `OVERFLOW` coming from?
here's the error:
```
In file included from C:/src/llvm-project/libc\src/__support/FPUtil/FEnvImpl.h:21:
C:/src/llvm-project/libc\src/__support/FPUtil/x86_64/FEnvImpl.h(271,44): error: expected unqualified-id
    raise_helper(internal::ExceptionFlags::OVERFLOW);
                                           ^
C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\ucrt\corecrt_math.h(965,25): note: expanded from macro 'OVERFLOW'
    #define OVERFLOW    _OVERFLOW
                        ^
C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\ucrt\corecrt_math.h(68,21): note: expanded from macro '_OVERFLOW'
#define _OVERFLOW   3   // overflow range error
```
If I had to guess it's coming in through `#include <fenv.h>`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134676