Comments on the removal of premake --with-doubles

[Remi Ricard]

Hi,

I don't if I was tired but it took me too much time to figure this out.

Before the premake was using the switch --with-doubles

If I understand correctly: now the premake is building both
the float and double library and they are put in their own /lib.

But I'm always using only one library so I want to build only the double
version.

There is no documentation to tell you. you must define dDOUBLE as a
preprocessor definition.

There is no message at compilation saying you are compiling this or that
version.

In the file include/ode/common.h there is

#if defined(dSINGLE)
typedef float dReal;
#ifdef dDOUBLE
#error You can only #define dSINGLE or dDOUBLE, not both.
#endif // dDOUBLE
#elif defined(dDOUBLE)
typedef double dReal;
#else
#error You must #define dSINGLE or dDOUBLE
#endif

BUT the you don't have to define something and the error will never show
up since in include/ode/odeconfig.h

There is those lines

#ifndef dDOUBLE
#ifndef dSINGLE
#define dSINGLE
#endif
#endif

If nothing is define dSINGLE is used

I think it should be better to display a message saying that
dDOUBLE or dSINGLE must be define than impose something.

i.e. removing the line following line in odeconfig.h
#ifndef dDOUBLE
#ifndef dSINGLE
#define dSINGLE
#endif
#endif

and replace then by

#ifndef dDOUBLE
#ifndef dSINGLE
#error You must #define dSINGLE or dDOUBLE as a preprocessor definition.
#endif
#endif

Remi

[David Walters]

> If nothing is define dSINGLE is used
>
> I think it should be better to display a message saying that
> dDOUBLE or dSINGLE must be define than impose something.
>

I believe that the intention was to impose a default set of working
parameters? Seems like a small thing to me.

> i.e. removing the line following line in odeconfig.h
> #ifndef dDOUBLE
> #ifndef dSINGLE
> #define dSINGLE
> #endif
> #endif
>

I'm fine with this as it's a really minor change that, but we'd better
ensure the automake build path is updated too as technically your proposing
a change that relies on the build system to allow the code to compile first
time with zero errors (even though it's hardly an "error" as the fix for
older "broken" builds is so trivial).

For premake, you can easily replicate the changes that add dDouble to the
project, to define dSingle in the opposite case and that will fix everyone
who is using the vanilla project files out of a release, which if you're not
then I think you can handle adding dSingle to the preprocessor section.

> and replace then by
>
> #ifndef dDOUBLE
> #ifndef dSINGLE
> #error You must #define dSINGLE or dDOUBLE as a preprocessor definition.
> #endif
> #endif
>

I don't think you need to add this bit - common.h is a public header and is
already doing the job of detecting that neither is present. For completeness
you might want to double check that the demos aren't being naughty and
#including select bits from "include/ODE" - which may bypass this check?

Regards,
Dave

[Remi Ricard]

David Walters wrote:
>> If nothing is define dSINGLE is used
>>
>> I think it should be better to display a message saying that
>> dDOUBLE or dSINGLE must be define than impose something.
>>
>>
>
> I believe that the intention was to impose a default set of working
> parameters? Seems like a small thing to me.
>

Or ode can say something like "dSINGLE and dDOUBLE not defined by the
user using the default dSINGLE".

>
>
>> i.e. removing the line following line in odeconfig.h
>> #ifndef dDOUBLE
>> #ifndef dSINGLE
>> #define dSINGLE
>> #endif
>> #endif
>>
>>
>
> I'm fine with this as it's a really minor change that, but we'd better
> ensure the automake build path is updated too as technically your proposing
> a change that relies on the build system to allow the code to compile first
> time with zero errors (even though it's hardly an "error" as the fix for
> older "broken" builds is so trivial).
>

I will check if automake does add dDOUBLE or dSINGLE

> For premake, you can easily replicate the changes that add dDouble to the
> project, to define dSingle in the opposite case and that will fix everyone
> who is using the vanilla project files out of a release, which if you're not
> then I think you can handle adding dSingle to the preprocessor section.
>

The dDOUBLE is added by the premake to the preprocessor section of VC++
but I don't know for
dSINGLE. I will check if dSINGLE is added.

The problem arise if someone (like me) is not using the project
generated by premake or automake. (Our make system had problem dealing
with the ode directory tree).
You can easily miss the fact that dSINGLE or dDOUBLE must be added to as
a preprocessor flag.
This is why I was suggesting the following line which will tell you you
forgot to configure something.

For dTRIMESH_OPCODE there is a switch for premake so it is easier to
know that you have a choice to add this to the config.h file.

>
>
>> and replace then by
>>
>> #ifndef dDOUBLE
>> #ifndef dSINGLE
>> #error You must #define dSINGLE or dDOUBLE as a preprocessor definition.
>> #endif
>> #endif
>>
>>
>
> I don't think you need to add this bit - common.h is a public header and is
> already doing the job of detecting that neither is present.

It has detected that neither is present but it does not tell you.

Maybe something like this instead which will no stop the compilation.

#ifndef dDOUBLE
#ifndef dSINGLE
#define dSINGLE

#prama message("Using the default value dSINGLE since neither dSINGLE or dDOUBLE was found in the preprocessor definition list".)
#endif
#endif

If dDOUBLE or dSINGLE is not defined you will get many message but you have an indication on how to remove the "compilation warning".

> For completeness
> you might want to double check that the demos aren't being naughty and
> #including select bits from "include/ODE" - which may bypass this check?
>

I will try to check it.

Remi