Update min-max to GET_MIN-GET_MAX #3799

obastemur · 2017-09-25T17:51:40Z

Xcode9 undefs min-max.

obastemur · 2017-09-25T17:55:42Z

To prevent min max usage going forward, undef / assert(false) both min and max for ChakraCore builds.

mmitche · 2017-09-25T18:56:52Z

@dotnet-bot test this please

MikeHolman · 2017-09-25T19:10:03Z

lib/Common/Core/CommonMinMax.h

 template<class T> inline
 _Post_equal_to_(a > b ? a : b) _Post_satisfies_(return >= a && return >= b)
-    const T& max(const T& a, const T& b) { return a > b ? a : b; }
+    const T& GET_MAX(const T& a, const T& b) { return a > b ? a : b; }


nit: the casing kind of implies that this is a macro. I feel like a GetMax or just Max would be better

@MikeHolman agree. I would gladly consider GetMax or Max at the first place. Well, such a large project + dependencies. GET_MAX is kinda different enough to define at global scale?

Updating the original definitions to GetM**Global and re-defining back as GET_M**

#define GET_MAX GetMaximumGlobal #define GET_MIN GetMinimumGlobal

So, this should work for anyone is searching for define GET_MAX project wide

mmitche · 2017-09-25T19:55:54Z

@obastemur There seems to be some issue with the win2k8 machines connected to the upgraded Jenkins (they don't support newer TLS versions). I'm seeing what I can do about it

obastemur · 2017-09-26T18:37:37Z

@dotnet-bot test Windows 7 ci_dev12_x64_debug please

obastemur · 2017-09-26T18:51:20Z

@MikeHolman @digitalinfinity additional thoughts?

jianchun · 2017-09-26T21:14:19Z

@obastemur Curious, why Xcode undefs min/max forbids this project to use min/max? As you've seen we were using our own min/max functions in "CommonMinMax.h" and not affected by the macros?

obastemur · 2017-09-27T13:12:05Z

@obastemur Curious, why Xcode undefs min/max forbids this project to use min/max?

IMHO it's a good practice that they enforced this with latest xcode. It's not just this project for sure.

As you've seen we were using our own min/max functions in "CommonMinMax.h" and not affected by the macros?

@jianchun I guess you missed pal.h?

Well, I could do the same on pal.h and trick the compiler hence keep using min/max vs ..... this PR and make it right once for all? Well, if you are not suggesting min/max global def.. is a good choice anyway?

and..
#3799 (comment)

jianchun · 2017-09-27T17:04:46Z

IMO getting rid of the macro is a good thing; removing the paradigm is unnecessary. STL kept std::min/std::max. ChakraCore already had the solution in place -- CommonMinMax.h. ChakraCore does not use the macro. Why changing hundreds of files to solve a non-exist problem? Fixing pal suffices.

obastemur · 2017-09-27T17:15:22Z

@jianchun well, std::max != max.. if you suggest that we could put it under a namespace , wouldn't we change all those?

Otherwise, how is different max as a global definition vs global function? Well, one of them you can't catch with ifdef ?

Probably my comment wasn't clear but from the possible solutions, I picked using a unique name instead of tricking the compiler. If you have other name suggestion, go ahead!.. if you suggest that we may put max/min under a namespace, I'm also in..

obastemur · 2017-09-27T17:26:35Z

Ref file : https://github.com/chapuni/libcxx/blob/master/include/__undef_min_max

Once again, this is all about macro min is incompatible with C++. Try #define NOMINMAX " "before any Windows header. #undefing min ...

However, using this opportunity, I made max/min related function/macro is differentiating.

jianchun · 2017-09-27T18:33:23Z

@obastemur Again, ChakaCore had already solved the "bad macro" problem: "NOMINMAX" defined globally in Build props; replaced with "CommonMinMax.h" functions. Existing "CommonMinMax.h" was already preventing the macro -- would already be a build failure if the macro kicks in. Thus we need nothing to "trick" the compiler. The macro simply did not exist and were not allowed in ChakraCore already.

My debate point is that the function names "min/max" are fine and no need to replace with custom ones. I think they are well known and better than custom ones. The custom ones make code slightly harder to read.

That's a minor point and I'm not blocking this PR.

obastemur · 2017-09-27T19:11:28Z

@jianchun I'm also not happy with the size of change either (considering the mess I'm going to deal with during master branch merge conflicts)

The macro simply did not exist and were not allowed in ChakraCore already.

I agree it doesn't on ChakraCore libs but it does on PAL. My question/concern is that global macro min/max is bad but function max is okay? IMHO not.

That's a minor point and I'm not blocking this PR.

I do appreciate the discussion and not strong on pushing this change as is. Because of (as you said) ..I think they are well known and better than custom ones.

jianchun · 2017-09-27T19:52:33Z

My question/concern is that global macro min/max is bad but function max is okay? IMHO not.

What's your concern with global function min/max? (The macros are bad because they have side effects. Your quote macro min is incompatible with C++ seems to refer that the macros conflict with std::min/std::max. These do not apply to our global function min/max. Ours seem equivalent to std::min/std::max except we don't require a namespace because we use them internally.)

obastemur · 2017-09-28T17:02:39Z

@jianchun my concerns are

1 - we have bunch of min max variable names all over the place.
2 - although the function names are known, they are short, simple, used all over the place hence not a good practice? (if they were under a sep. namespace, that's another story)
3 - template function is c++ only, considering PAL has C part too, long term no min/max as is there. Although no consumer on c land.. still.

However, I understand the overall concerns and agree with some of them. Well, the intention here is to fix the things. not break :)

I will make the necessary PAL side changes only. updating the PR. Thanks for the discussion.

jianchun

Thanks for the discussion and update!

Merge pull request #3799 from obastemur:xcode9 Xcode9 undefs min-max.

msftclas added the cla-already-signed label Sep 25, 2017

MikeHolman reviewed Sep 25, 2017

View reviewed changes

obastemur force-pushed the xcode9 branch 2 times, most recently from c3d20a9 to 434ea0d Compare September 26, 2017 11:53

msftgits removed the cla-already-signed label Sep 26, 2017

obastemur force-pushed the xcode9 branch from 434ea0d to f20fb70 Compare September 26, 2017 12:22

obastemur mentioned this pull request Sep 26, 2017

build: execute fail with Abort trap: 6 in macOS with Xcode 9 nodejs/node-chakracore#403

Closed

MikeHolman approved these changes Sep 26, 2017

View reviewed changes

jackhorton mentioned this pull request Sep 28, 2017

Intl-ICU: implement Intl.NumberFormat under ICU #3809

Merged

obastemur force-pushed the xcode9 branch from f20fb70 to 8558c02 Compare September 28, 2017 17:59

xplat: fix xcode9 build break

7d654f3

obastemur force-pushed the xcode9 branch from 8558c02 to 7d654f3 Compare September 28, 2017 18:03

jianchun approved these changes Sep 28, 2017

View reviewed changes

dilijev approved these changes Sep 28, 2017

View reviewed changes

chakrabot merged commit 7d654f3 into chakra-core:release/1.7 Sep 29, 2017

chakrabot pushed a commit that referenced this pull request Sep 29, 2017

[MERGE #3799 @obastemur] Update min-max to GET_MIN-GET_MAX

61d8817

Merge pull request #3799 from obastemur:xcode9 Xcode9 undefs min-max.

chakrabot pushed a commit that referenced this pull request Sep 29, 2017

[1.7>master] [MERGE #3799 @obastemur] Update min-max to GET_MIN-GET_MAX

e096c45

Merge pull request #3799 from obastemur:xcode9 Xcode9 undefs min-max.

Update min-max to GET_MIN-GET_MAX #3799

Update min-max to GET_MIN-GET_MAX #3799

Uh oh!

Conversation

obastemur commented Sep 25, 2017

Uh oh!

obastemur commented Sep 25, 2017

Uh oh!

mmitche commented Sep 25, 2017

Uh oh!

MikeHolman Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

mmitche commented Sep 25, 2017

Uh oh!

obastemur commented Sep 26, 2017

Uh oh!

obastemur commented Sep 26, 2017

Uh oh!

jianchun commented Sep 26, 2017

Uh oh!

obastemur commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jianchun commented Sep 27, 2017

Uh oh!

obastemur commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obastemur commented Sep 27, 2017

Uh oh!

jianchun commented Sep 27, 2017

Uh oh!

obastemur commented Sep 27, 2017

Uh oh!

jianchun commented Sep 27, 2017

Uh oh!

obastemur commented Sep 28, 2017

Uh oh!

jianchun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

MikeHolman Sep 25, 2017 •

edited

Loading

obastemur commented Sep 27, 2017 •

edited

Loading

obastemur commented Sep 27, 2017 •

edited

Loading