Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cmakedefine01 instead of cmakedefine #641

Merged
merged 1 commit into from
Mar 28, 2016
Merged

Use cmakedefine01 instead of cmakedefine #641

merged 1 commit into from
Mar 28, 2016

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Mar 20, 2016

cmakedefine defines only when the cmake variable is set to true or 1 in cmake script, whereas cmakedefine01 always defines either #define VAR 1 or #define VAR 0 depending on the variable. cmakedefine will go along with #ifdef VAR, and cmakedefine01 will go along with #if VAR.

This might be arguable, but #if VAR seems to be more intuitive to use to me. I would like to know other's thoughts.

@jslee02 jslee02 added this to the DART 6.0.0 milestone Mar 20, 2016
@mkoval
Copy link
Collaborator

mkoval commented Mar 20, 2016

👍 Makes sense to me.

@psigen
Copy link
Collaborator

psigen commented Mar 21, 2016

If dealing with boolean situations, I tend to prefer #if defined(VAR) to #ifdef VAR or #if VAR for the following reasons:

  • #if defined(VAR) is categorically more flexible than #ifdef VAR because it allows compound conditionals (which #if VAR also supports).
  • #if VAR requires that VAR have some value if -Wundef is in use. This means that the actual compiler flags become polluted with a huge list like -DBUILD_TYPE_DEBUG=0 -DBUILD_TYPE_RELEASE=0 -DBUILD_TYPE_RELEASE=0 -DBUILD_TYPE_RELWITHDEBINFO=1 because it has to include every option that isn't in use. I generally don't like the idea of having combinatorial amounts of command line flags for each exclusive option set.

Willing to hear other opinions though.

@mxgrey
Copy link
Member

mxgrey commented Mar 21, 2016

I'm most in favor of @psigen 's proposal. Explicitly defining every command line variable could result in some hideous output if we ever need to investigate build issues.

@jslee02
Copy link
Member Author

jslee02 commented Mar 24, 2016

If dealing with boolean situations, I tend to prefer #if defined(VAR) to #ifdef VAR or #if VAR for the following reasons:

To clarify, do you mean to prefer to use #cmakedefine VAR 1 with #if defined(VAR)?

This means that the actual compiler flags become polluted with a huge list like -DBUILD_TYPE_DEBUG=0 -DBUILD_TYPE_RELEASE=0 -DBUILD_TYPE_RELEASE=0 -DBUILD_TYPE_RELWITHDEBINFO=1 because it has to include every option that isn't in use.

Please correct me if something is wrong in the following. AFAIK, they are cmake variables rather than compiler flags. Those cmake variables are determined in cmake time based on CMAKE_BUILD_TYPE passed in with cmake command. CMAKE_BUILD_TYPE also can be omitted. In this case, it will be set to Release by default.

The actual preprocessors are then configured in Config.h.in in cmake time based on the cmake variables. The only choice to be made by us is which one is preferred among:

/* #undef BUILD_TYPE_DEBUG */
#define BUILD_TYPE_RELEASE 1
/* #undef BUILD_TYPE_RELWITHDEBINFO */
/* #undef BUILD_TYPE_MINSIZEREL */

#define HAVE_NLOPT 1
#define HAVE_IPOPT 1
/* #undef HAVE_SNOPT */
#define HAVE_BULLET_COLLISION 1

#define DART_USE_FCLMESHCOLLISIONDETECTOR 0

and

#define BUILD_TYPE_DEBUG 0
#define BUILD_TYPE_RELEASE 1
#define BUILD_TYPE_RELWITHDEBINFO 0
#define BUILD_TYPE_MINSIZEREL 0

#define HAVE_NLOPT 1
#define HAVE_IPOPT 1
#define HAVE_SNOPT 0
#define HAVE_BULLET_COLLISION 1

#define DART_USE_FCLMESHCOLLISIONDETECTOR 0

All these will be automatically configured by cmake without polluting the compiler flags.

@jslee02
Copy link
Member Author

jslee02 commented Mar 27, 2016

As this doesn't pollute the compiler flags, looks good to merge?

@psigen
Copy link
Collaborator

psigen commented Mar 27, 2016

I think this is fine as these are strictly going into a header directly.

@mkoval and I discussed situations where someone might want to undefine something when compiling client code, but we concluded that it's a pretty rare case and we couldn't think of valid use-cases related to these definitions.

@jslee02
Copy link
Member Author

jslee02 commented Mar 28, 2016

Thanks! Merging now.

@jslee02 jslee02 merged commit 6cdbe4e into master Mar 28, 2016
@jslee02 jslee02 deleted the cmakedefine01 branch March 28, 2016 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants