Project

General

Profile

Bug #806

Get rid of #define min/max in macros.h

Added by Teemu Murtola over 8 years ago. Updated almost 6 years ago.

Status:
Closed
Priority:
High
Assignee:
-
Category:
core library
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

The header src/gromacs/legacyheaders/macros.h #defines symbols min and max, which causes constant headache when compiling C++ source files. This is because quite a few standard C++ headers include <algorithm>, which defines these functions using templates. So if macros.h has been included before C++ headers, one quite often gets confusing error messages. Currently, I've worked around those by adding #undef statements in the source files (all should be marked with // FIXME), but a better solution should be found.


Related issues

Related to GROMACS - Bug #852: Jenkins Build for masterClosed12/03/2011

Associated revisions

Revision 6073a8b7 (diff)
Added by Roland Schulz about 8 years ago

Fix name collision of min/max with C++ std::min/std::max

min/max is defined as the macro for C compililation and
as std::min/std::max for C++ compilation. New C++ code should not depend
on this so removed macros.h from other headers and added it where
needed. Removed hacks from existing C++ files where they were no longer
necessary.

Fixes issue #806.

Change-Id: I612d00e092ef4979a5a50b4e2d56fb767a04041e

History

#1 Updated by Teemu Murtola over 8 years ago

It actually seems that vec.h doesn't require macros.h, so the include statement could be simply removed from there. But several other headers quite likely include it as well, and some of them may actually need it. It also appears that quite a few files fail to compile if min/max are simply removed from macros.h. An #ifndef __cplusplus around them would break C++ compilation of C source files, so that is not ideal either. Nor is complementing it with #ifdef __cplusplus with alternative definitions, since there already exists std::min and std::max.

One solution that would not require larger changes to existing code could be to move the macros to a separate header, e.g., minmax.h. This header should not be included from any headers, but only from source files that actually need it. Also, it should not be used in new code, so that perhaps at some point it could be completely removed.

In the long run, quite a few of the definitions in macros.h don't seem to be very useful, so perhaps we should try to remove the whole header.

Some of the // FIXME hacks mentioned in the description are still in commits pending review in Gerrit, so it would be best to wait for them to do all the fixes at the same time.

#2 Updated by Teemu Murtola about 8 years ago

I think that now all pending changes with those FIXME issues have been reviewed and merged, so this issue could be tackled as a whole.

I also noticed that source:src/gromacs/selection/params.cpp redefined max and min using templates, that should also be fixed to use std::min and std::max.

#3 Updated by Roland Schulz about 8 years ago

I uploaded a proposed fix at https://gerrit.gromacs.org/318

#4 Updated by Teemu Murtola about 8 years ago

  • Status changed from New to Closed

Mentioned change has been merged to master after some revisions.

#5 Updated by Teemu Murtola almost 6 years ago

  • Project changed from Source code reorganization to GROMACS
  • Category set to core library
  • Affected version set to 5.0

Also available in: Atom PDF