Skip to content

Windows compatibility (new PR) - #526

Draft
andrewxu319 wants to merge 6 commits into
developfrom
andrew/windows-compatibility-v2
Draft

Windows compatibility (new PR)#526
andrewxu319 wants to merge 6 commits into
developfrom
andrew/windows-compatibility-v2

Conversation

@andrewxu319

Copy link
Copy Markdown
Collaborator

Description

A continuation of #445. I started a new branch/PR because of rebasing issues in the old one. I've also addressed/responded to all comments on the old PR.

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.

Make compiler-specific changes such that GridKit runs and pass all tests on Windows. Specifically, it now works with MinGW G++, LLVM/Clang, and MSVC.

Closes #(issue)

Mentions @(user)

Proposed changes

Describe how your changes here address the issue and why the proposed changes
should be accepted.

  • Append .string() to .filename() to extract the underlying string if the OS is Windows
  • Replace __attribute__((always_inline)) inline with a compiler-specific FORCE_INLINE macro, defined in root CMakeLists.txt (it seems to have been added in an earlier commit then removed. I added it back)
  • Inline Utilities::toUpper() and Utilities::strip to avoid ODR-related errors
  • Update examples/Consumer/CMakeLists.txt and create a PowerShell script so that test_install works on Windows

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the 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.

All tests pass and make test_install works on all three compilers. On MSVC, use cmake --build . --target test_install instead of make test_install.

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • [N/A] There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.
  • I have updated CHANGELOG.md to reflect the changes in this PR. If this is a minor PR that is part of a larger fix already included in the file, state so.

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant