nfrechette / rtm

Realtime Math
MIT License
732 stars 44 forks source link

fix: std::numeric_limits::max() in memory_utils.h conflicted with minwindef.h #186

Closed Kethers closed 11 months ago

Kethers commented 11 months ago

fix: usage of std::numeric_limits::max() in memory_utils.h conflicted with minwindef.h under some circumstances

Hi, I use rtm in my personal project. Somehow I include the "minwindef.h" file (actually I don't know where it's included, maybe it's included by some third-party library I am using) and when I build project on Windows, an error happen:

0>memory_utils.h(208): Warning C4003 : 类函数宏的调用“max”参数不足 // means that number of parameters not correct when calling macro max
0>memory_utils.h(208): Error C1075 fatal: “{”: 未找到匹配令牌

Line 208: return uint64_t(input) <= uint64_t((std::numeric_limits<DstType>::max()); I jump to source code of max and it turns out to be in minwindef.h:

#ifndef max
#define max(a,b)            (((a) > (b)) ? (a) : (b))
#endif

With google it's easy to find out solution: https://stackoverflow.com/questions/1394132/macro-and-member-function-conflict.

It's a tricky and potential build error only happens in specific situation. I try to fix it in this PR, and maybe there's a better way?

commit-lint[bot] commented 11 months ago

Contributors

Kethers

Commit-Lint commands
You can trigger Commit-Lint actions by commenting on this PR: - `@Commit-Lint merge patch` will merge dependabot PR on "patch" versions (X.X.Y - Y change) - `@Commit-Lint merge minor` will merge dependabot PR on "minor" versions (X.Y.Y - Y change) - `@Commit-Lint merge major` will merge dependabot PR on "major" versions (Y.Y.Y - Y change) - `@Commit-Lint merge disable` will desactivate merge dependabot PR - `@Commit-Lint review` will approve dependabot PR - `@Commit-Lint stop review` will stop approve dependabot PR
CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

nfrechette commented 11 months ago

Thank you for the contribution!

Windows does define min/max macros which are typically incompatible with the C++ standard library as it includes a number of functions with those names. :( Usually, in my experience, most modern applications that include windows header with #define NOMINMAX before including it.

But here, it seems that the fix is easy enough. I'll see if I can reproduce your issue locally to validate the fix as well.

nfrechette commented 11 months ago

I was able to reproduce locally. I'll add a check to make sure that this issue is checked by CI to avoid it coming back in the future. Thanks!

nfrechette commented 11 months ago

@all-contributors add @Kethers as code

allcontributors[bot] commented 11 months ago

@nfrechette

I've put up a pull request to add @Kethers! :tada: