Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work around function std name collision in MSVC #1679

Closed
wants to merge 2 commits into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Dec 9, 2024

Refs #1513.

There is a bug in MSVC that, if the std function is exposed to current scope with using namespace mlx::core, all following std::xxx invocations that try to access the namespace std will fail with very unhelpful errors:

C:\cygwin\home\zcbenz\codes\mlx\python\src\array.cpp(28,18): error C2874: using-de
claration causes a multiple declaration of 'std' [C:\cygwin\home\zcbenz\codes\mlx\
build\temp.win-amd64-cpython-311\Release\mlx.core\python\src\core.vcxproj]
      C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.
  34433\include\numeric(22,1):
      see declaration of 'std'
C:\Users\zcbenz\AppData\Roaming\Python\Python311\site-packages\nanobind\include\na
nobind\nb_cast.h(160,27): error C2872: 'std': ambiguous symbol [C:\cygwin\home\zcb
enz\codes\mlx\build\temp.win-amd64-cpython-311\Release\mlx.core\python\src\core.vc
xproj]
  (compiling source file '../../../../../../python/src/convert.cpp')
      C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.42.
  34433\include\numeric(22,1):
      could be 'std'
      C:\Users\zcbenz\AppData\Roaming\Python\Python311\site-packages\nanobind\incl
  ude\nanobind\nb_cast.h(160,27):
      or       'std'

(MSVC thinks std:: is ambiguous.)

There are 2 solutions, one taken by this PR is to rename the std function to std_dev and use it internally, while adding a std alias only available in public mlx.h header. It makes minimum changes to existing code with the cost of adding a new header.

The other solution would be replacing the using namespace mlx::core with namespace mx = mlx::core and use mx::xxx() instead of xxx() in the python bindings code. I prefer this one but it would result in a relatively large PR.

@awni
Copy link
Member

awni commented Dec 9, 2024

I kind of prefer the second solution as well ... it feels like the right way to fix name collisions and how the C++ namespace should be used in user code. It is a big and tedious diff though 😓

@zcbenz
Copy link
Contributor Author

zcbenz commented Dec 10, 2024

Will go the 2nd solution, feels like the right way to go.

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.

2 participants