Make Windows (MinGW G++) compatible#445
Conversation
…As of this comment, the code works on MinGW g++.
* Fix phasor_dynamics_systemmodel targets. --------- Co-authored-by: nkoukpaizan <nkoukpaizan@users.noreply.github.com>
| ```powershell | ||
| mkdir build | ||
| cd build | ||
| cmake -DCMAKE_INSTALL_PREFIX=/path/to/install -DBUILD_SHARED_LIBS=OFF ../ |
There was a problem hiding this comment.
Are different instructions really needed? The only difference I see with the earlier instructions is DBUILD_SHARED_LIBS=Off. I would recommend implementing a workaround for that in CMake instead.
There was a problem hiding this comment.
Maybe not. Removed it for now. However I did also need to specify the directory ../ instead of ../GridKit.
There was a problem hiding this comment.
../GridKit is suggested for out-of-source builds, with the build and GridKit directories being at the same level in the file structure.
This reverts commit a5d7e80.
…t into andrew/windows-compatibility
|
re 5ffbf8d: I recommend rebasing ( |
…t into andrew/windows-compatibility
…As of this comment, the code works on MinGW g++.
This reverts commit a5d7e80.
…t into andrew/windows-compatibility
Rebase done. |
superwhiskers
left a comment
superwhiskers
left a comment
There was a problem hiding this comment.
i don't have a windows machine to test this on
| #elif defined(_MSC_VER) | ||
| #define FORCE_INLINE [[msvc::forceinline]] inline | ||
| #else | ||
| #define FORCE_INLINE __attribute__((always_inline)) inline |
There was a problem hiding this comment.
if you used the standard syntax for attributes, such as [[gnu::always_inline]] instead of __attribute__((always_inline)), this macro wouldn't be necessary as you could write [[msvc::forceinline, gnu::always_inline]].
There was a problem hiding this comment.
at least, in my testing w/ clang, clang ignores the msvc::forceinline, raising a warning but compiles just fine. perhaps msvc doesn't like seeing gnu::always_inline, but that would be weird. could just ignore that warning via flags or something like that to reduce noise. alternatively, keep the conditional macro but using the standard syntax anyway.
| * @brief Convert a string to all uppercase | ||
| */ | ||
| std::string toUpper(std::string str) | ||
| inline std::string toUpper(std::string str) |
There was a problem hiding this comment.
what compiler error warranted this?
| - Added off-nominal tap ratio and phase shift support to the PhasorDynamics `Branch` model. | ||
| - Added portable Vector class to GridKit | ||
| - Added portable Vector class to GridKit. | ||
| - Added Windows compatibility. |
There was a problem hiding this comment.
specify it's mingw g++ only or list caveats with msvc. also consider testing llvm on windows as well (uncommon, but would allow enzyme to work, maybe).
3 participants
Description
Make compiler-specific changes such that GridKit runs and pass all tests on Windows. Specifically, it now works with MinGW G++, but not MSVC.
Please describe the issue that is addressed (bug, new feature,
documentation, enhancement, etc.). Please also include relevant motivation and
context. List any dependencies that are required for this change.
Closes #(issue)
Mentions @(user)
Proposed changes
Describe how your changes here address the issue and why the proposed changes
should be accepted.
.string()to.filename()to extract the underlying string if the OS is Windows__attribute__((always_inline)) inlinewith a compiler-specificFORCE_INLINEmacrotoUpper()to avoid compiler errorsChecklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here
to help! This is simply a reminder of what we are going to look for before
merging your code.
-Wall -Wpedantic -Wconversion -Wextra.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining
why you chose the solution you did and what alternatives you considered, etc...