Menu ▾ ▴

#9 min()/max() cleanup

open
Klauss++
None
5
2013-07-27
2011-07-04
Anonymous
No

Hello,

searching the VS source code I stumbled upon multiple copies of min/max macros and functions.
I thought it could take a little cleanup pass to consolidate those. The resulting diffstat is nice:

20 files changed, 166 insertions(+), 242 deletions(-)

I tried to be careful, and not to go too far, what may have changed is inlining for some
cases (should be harmful). Also "static" keyword removed in some other cases.

Please review & maybe apply.

Any comments appreciated, especially if this kind of changes are appreciated, or if
I should go one step further and consolidate harder...

Discussion

  • Comment has been marked as spam. 
    Undo

    View and moderate all "patches Discussion" comments posted by this user

    Mark all as spam, and block user from posting to "Patches"

    Anonymous

    Anonymous - 2011-07-04

    Forgot to mention: compiled OK, ran the game, bought some contraband, flown & jumped sector, docked...

     

  • Klauss++

    Klauss++ - 2011-07-05
    • assigned_to: nobody --> klaussfreire
     

  • Klauss++

    Klauss++ - 2011-07-05

    In general, the patch seems nice.
    But: since we're consolidating, instead of having vs_fmax/vs_uimax/vs_tmax, why not have a single template vs_max and be done?

    In most cases, type inference should do its work. And when necessary, vs_max<type> can explicitly specify the type.

    Also, I wouldn't touch the source in opcode (csbox), that's a third-party library we've imported into our source tree. Though it's patched when necessary, it's best not to introduce unnecessary modifications (to make applications of upstream patches easier)

     

  • Klauss++

    Klauss++ - 2011-07-21

    Did you have a chance to make the changes I requested?
    I don't think I have the time to make them myself, but if you do, I'll most likely commit the patch.

     
  • Comment has been marked as spam. 
    Undo

    View and moderate all "patches Discussion" comments posted by this user

    Mark all as spam, and block user from posting to "Patches"

    Anonymous

    Anonymous - 2011-07-23

    I revisited the issue, now I think the ultimate goal would be to
    use STL std::min() & std::max() everywhere, as in the newly attached patch (which ignores opbox.cpp)

    please review carefully, as I had to add some casting here and there to let it compile properly

    diffstat looks quite nicely negative:
    18 files changed, 95 insertions(+), 224 deletions(-)

     
  • Comment has been marked as spam. 
    Undo

    View and moderate all "patches Discussion" comments posted by this user

    Mark all as spam, and block user from posting to "Patches"

    Anonymous

    Anonymous - 2011-07-23

    Compiled and run for a few minutes under linux, I think a windows build & quick test would be useful...

    As to opbox.cpp any pointer to give upstream the same kind of cleanup ?
    Where does this code come from ?

     

  • Klauss++

    Klauss++ - 2011-07-25

    I tried the std::min/max thing myself a while ago, and the problem is windows compilers break on std::min/max. Something related to some macros defined in windows.h that borks it all up.

    Maybe modern compilers have fixed that, if you want to go for std:: stuff, please contact some of the windows guys in the forums and confirm that it does build in windows.

    Otherwise, I'd prefer our own vsmin/max, they wouldn't collide with windows.

    About opcode, it all comes from here: http://www.codercorner.com/Opcode.htm

     

  • Vincent Legoll

    Vincent Legoll - 2013-07-14

    Here is an updated patch, I'm running with this on linux, x86_64, gcc-4.7, looks like it's working. I'll let it bake some more time.

    Someone on win could try it ?
    Report status here, please.

     
    patch-std-minmax.patch

  • Klauss++

    Klauss++ - 2013-07-27

    Thing with std::min/max is that they break in windows, because they define min/max to be some macro.

    What you could do, is define a MIN and MAX (all caps) that is a preprocessor define that resolves to what opcode uses some template function vsmin/vsmax in windows, and std::min/max in linux.

    That'd be portable enough.

    I'd leave opcode alone. Opcode's definition of MIN/MAX is nice and all, but causes some badness when you use it with complex expressions inside (it evaluates them multiple times).

     

  • Klauss++

    Klauss++ - 2013-07-27

    Thinking twice, I think MIN/MAX may also be defined in windows headers. Do you know anything about it pheonixstorm?

     

Log in to post a comment.