Skip to content

Commit

Permalink
Fix missing #include
Browse files Browse the repository at this point in the history
  • Loading branch information
@PilzAdam
PilzAdam committed Oct 27, 2015
1 parent 27eed13 commit c406438
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/basicmacros.h
View file
Expand Up @@ -20,6 +20,8 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#ifndef BASICMACROS_HEADER
#define BASICMACROS_HEADER

#include <algorithm>

#define ARRLEN(x) (sizeof(x) / sizeof((x)[0]))

#define MYMIN(a, b) ((a) < (b) ? (a) : (b))
Expand Down

9 comments on commit c406438

@JohnnyComeL8ly
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, PilzAdam!

@ShadowNinja
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be included in each file that uses CONTAINS since it's only used in a few places and you shouldn't bloat a basic header like this so much for one macro. Also, see 27eed13#commitcomment-14018111 .

@est31
Copy link
Contributor

@est31 est31 commented on c406438 Oct 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShadowNinja I don't care whether its this way or that way, but it should build.

@ShadowNinja
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@est31: Of course...

@kwolekr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I guess including here is fine as a temporary fix so the master branch compiles, but it does need to be fixed the right way. What platform does it not compile under, exactly?

@est31
Copy link
Contributor

@est31 est31 commented on c406438 Oct 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both ways are "right". One is just more prefferable as it spares includes in some files.

@PilzAdam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwolekr I don't insist on this being solved this way. I would prefer this to stay like this, since it looks cleaner to me, but if compile time is more important to you then I won't object changing it.

As far as I have seen it didn't compile on any platform.

@kwolekr
Copy link
Contributor

@kwolekr kwolekr commented on c406438 Nov 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't have pushed this if it failed to compile. It worked for me and it even compiled on all the platforms tested by Travis/Jenkins.

@PilzAdam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwolekr this is the build for the commit in question: https://travis-ci.org/minetest/minetest/builds/87610357

Note that all builds failed.

Please sign in to comment.