Because I cannot attach a text file in github.


The file error.txt is from clang (it stopped before going to far)

icerror.txt is from Intel compiler.

gcc_error.txt is from g++ 4.8

All of them when trying the branch hotfix/github_issue_29


-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'gcc_error.txt' (390 KB) Download 'icerror.txt' (179 KB) Download 'error.txt' (81 KB)

Am 29.06.2014 11:52, schrieb Le_Forgeron:

--------
./base/colour.h: In instantiation of ‘T 
pov_base::GenericColour<T>::MaxAbs() const [with T = float]’:
./base/colour.h:1422:27:   required from ‘T 
pov_base::GenericColour<T>::WeightMaxAbs() const [with T = float]’
backend/render/trace.cpp:999:34:   required from here
./base/colour.h:1456:54: error: no matching function for call to 
‘max(float&, double)’
                  result = max(result, fabs(mColour[i]));
--------

That's a quite interesting error we have here: With T=float, mColour is 
defined as "float mColour[3]", so the compiler should pick "float 
fabs(float)", but the max() signature it is trying to find a match for 
indicates that the compiler rather chooses "double fabs(double)".

So either...:

(A) The compiler is buggy, or

(B) The header files are buggy, declaring "fabs(float)" as returning a 
double instead, in obvious violation of the C++03 standard, or

(C) The header files are buggy, failing to declare "fabs(float)" at all, 
again in obvious violation of the C++03 standard.

So we should probably contact the authors of g++ to fix this...


... or should we? Having had a quick glance at the C++03 standard, 
thruth gave me a roundhouse kick right in the face:

(D) We're not including the right header in the first place.


Just a few weeks ago, I had done some work on work coding style; one 
thing I addressed was the order in which header files should be 
included, as well as which names should be used for C standard headers. 
One thing that had been bugging me most was the inconsistenty in using 
the old C standard header file names vs. the new C++ names (e.g. 
<stdlib.h> vs. <cstdlib>), and I went for the C++ names for the coding 
rules; it had been bugging me to the degree that in all the files I have 
been touching ever since, I replaced all the C header names with the C++ 
names.

Well, it should probably have bugged me even more.

 From ISO-IEC 14882-2003 (aka C++03):
-------------------------------------------------
26 Numerics library
[...]
26.5 C Library

Tables 80 and 81 describe headers <cmath> and <cstdlib> [...], respectively.
[...]
The contents of these headers are the same as the Standard C library 
headers <math.h> and <stdlib.h> respectively, with the following additions:
[...]
     float fabs (float);
[...]
-------------------------------------------------

Read this again, and let it sink in: <cmath> is /not/ a C++ canonical 
name for <math.h>. It is an entirely different library, with /added/ 
functionality over <math.h>.

Guess which version of <math.h> / <cmath.h> we've been including in 
POV-Ray all the time...


I think it's time to kick out all the C standard header files for good. Now.

Post a reply to this message

Le 30/06/2014 14:25, clipka a écrit :
> Having had a quick glance at the C++03 standard, thruth gave me a
> roundhouse kick right in the face:

I hope you are not too much injured.

Nicely spotted, btw.

-- 
Just because nobody complains does not mean all parachutes are perfect.

Post a reply to this message

Second round, on f42a95b668458bfd8bf626519669b3cdc36f0d3c

* still std::numeric_limits<> but now in base/types.h (nned <limits>)
* texture.cpp, 1266 : default shared_ptr is to nullptr, not worth setting ?
* same for normal.cpp, 593.
* base/colour.cpp:53:22: error: specializing member
‘pov_base::GenericColour<float>::mkDefaultWavelengths’ requires
‘template<>’ syntax
     const MathColour MathColour::mkDefaultWavelengths =
MathColour(RGBColour(0.70, 0.52, 0.48));

* "return GenericPigmentBlendMapPtr(NULL);" might compile better as
"return GenericPigmentBlendMapPtr()" (calling the constructor of
shared_ptr without argument, making a shared_ptr to nullptr.
(otherwise, IIRC, NULL is taken as the argument for the constructor of T
in shared_ptr<T>)

(I'm more familiar with C++11 std::shared_ptr<T>, so I might be wrong
when applied to C++03 & boost . when I need fresh instanced value,
std::make_shared<T>(params for T's constructor...) is my friend)

Thanks for your update.
-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorgcc.txt' (428 KB) Download 'erroricpc.txt' (175 KB)

Am 30.06.2014 18:06, schrieb Le_Forgeron:

> * "return GenericPigmentBlendMapPtr(NULL);" might compile better as
> "return GenericPigmentBlendMapPtr()" (calling the constructor of
> shared_ptr without argument, making a shared_ptr to nullptr.
> (otherwise, IIRC, NULL is taken as the argument for the constructor of T
> in shared_ptr<T>)
>
> (I'm more familiar with C++11 std::shared_ptr<T>, so I might be wrong
> when applied to C++03 & boost . when I need fresh instanced value,
> std::make_shared<T>(params for T's constructor...) is my friend)

Technically you're right in that the NULL is not required, but you're 
wrong on the details: The shared_ptr<T> constructor does /not/ pass its 
arguments to a constructor of T; in fact, it never even constructs an 
instance of T at all. Instead, it takes a /pointer/ to an 
already-created instance of T as an argument (with NULL being a valid 
parameter as well). To construct a T without make_shared (which is not 
available in TR1), you have to invoke:

     shared_ptr<T>(new T(params for T's constructor...));

As for using "return shared_ptr<T>()" instead of "return 
shared_ptr<T>(NULL)", note that those pieces of code should never be 
called in the first place anyway (they all follow either an 
"assert(false)" or an "Error(...)"), and the only reason they even exist 
is to satisfy compilers ("all control paths must return a value"). I 
prefer to go for code clarity there and leave the "NULL" parameter in.

Post a reply to this message

Le 30/06/2014 19:20, clipka nous fit lire :
> Am 30.06.2014 18:06, schrieb Le_Forgeron:
> 
>> * "return GenericPigmentBlendMapPtr(NULL);" might compile better as
>> "return GenericPigmentBlendMapPtr()" (calling the constructor of
>> shared_ptr without argument, making a shared_ptr to nullptr.
>> (otherwise, IIRC, NULL is taken as the argument for the constructor of T
>> in shared_ptr<T>)
>>
>> (I'm more familiar with C++11 std::shared_ptr<T>, so I might be wrong
>> when applied to C++03 & boost . when I need fresh instanced value,
>> std::make_shared<T>(params for T's constructor...) is my friend)
> 
> Technically you're right in that the NULL is not required, but you're
> wrong on the details: The shared_ptr<T> constructor does /not/ pass its
> arguments to a constructor of T; in fact, it never even constructs an
> instance of T at all. Instead, it takes a /pointer/ to an
> already-created instance of T as an argument (with NULL being a valid
> parameter as well). To construct a T without make_shared (which is not
> available in TR1), you have to invoke:
> 
>     shared_ptr<T>(new T(params for T's constructor...));
> 
> As for using "return shared_ptr<T>()" instead of "return
> shared_ptr<T>(NULL)", note that those pieces of code should never be
> called in the first place anyway (they all follow either an
> "assert(false)" or an "Error(...)"), and the only reason they even exist
> is to satisfy compilers ("all control paths must return a value"). I
> prefer to go for code clarity there and leave the "NULL" parameter in.
> 

But it won't compile on gcc with NULL. :-/
(it's a reported error, not just a warning)

(as NULL is long int 0... and that's not transformable into a
shared_ptr<T>.) (nullptr seems fine for C++0x, but it's not in C++03;
does boost have it ?)

I stand corrected about the constructor syntax.

-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

I've seen the new commit (583d4c85e9357f06b02af41b41973a46f315fe3c)

It's better, half size of gcc complains, but does not compile yet.

* <limits> needed in safemath.h (for numeric_limits).. it crawls
* New->Blend_Map = NULL; ... same empty shared_ptr story (with
variations about return)
* base/colour.h , still the max() vs fabs() as double and float.



-- 
IQ of crossposters with FU: 100 / (number of groups)
IQ of crossposters without FU: 100 / (1 + number of groups)
IQ of multiposters: 100 / ( (number of groups) * (number of groups))

Post a reply to this message

Attachments:
Download 'errorgcc3.txt' (196 KB)

Am 30.06.2014 22:26, schrieb Le_Forgeron:
> I've seen the new commit (583d4c85e9357f06b02af41b41973a46f315fe3c)
>
> It's better, half size of gcc complains, but does not compile yet.
>
> * <limits> needed in safemath.h (for numeric_limits).. it crawls

Uh... no, it's not missing <limits>; that file has been included there 
for ages; it's missing the std:: namespace descriptor...

> * New->Blend_Map = NULL; ... same empty shared_ptr story (with
> variations about return)

Heh, that surplus piece of code is even in there twice... whoops.

> * base/colour.h , still the max() vs fabs() as double and float.

Now this starts getting me puzzled. Can you double-check whether we 
still have any reference to math.h in our code?

Post a reply to this message

Am 30.06.2014 23:33, schrieb clipka:

>> * base/colour.h , still the max() vs fabs() as double and float.
>
> Now this starts getting me puzzled. Can you double-check whether we
> still have any reference to math.h in our code?

I /think/ I can solve this riddle now:

While the C++03 standard is very ambiguous (at least to me) about 
whether the functions provided by <cmath> should live in the global or 
std:: namespace, the C++11 standard clarifies this, indicating that 
<cmath> /must/ provide the functions in the std:: namespace, and /may/ 
also provide them in the global namespace.

It appears to me that the GCC <cmath> header does the most confusing 
thing, by providing only /some/ functions in the global namespace - 
namely those for the "double" data type.


Ready for the next round.

Post a reply to this message

On 1-7-2014 1:43, clipka wrote:
> Ready for the next round.
>

Keep it going!

While this is all gobbledegook to me, I love to read your exchange of 
thoughts :-) It shows the brain at work... or is this play for you guys?

Thomas

Post a reply to this message