Bug 34538

Are there reasons we can't use std::complex other than the normal WebCore rule of not using stdlib types. If it is usable for your purposes, I think we should consider augmenting the rule for this, like we do for std::pair.

Created attachment 48062 [details] patch for Complex number class

Hi Sam, I anticipate extending this class in the future for various DSP algorithms, so I'd prefer to use our own implementation.

Comment on attachment 48062 [details] patch for Complex number class > + Complex() : m_real(0.0), m_imag(0.0) { } We try to keep abbreviations to a minimum in WebKit and use words instead. So this would normally be called "imaginary", not "imag". > + double abs() const { return sqrt(m_real*m_real + m_imag*m_imag); } We put spaces around operators like "*". > + double magnitude() const { return abs(); } Do we really need both the names "abs()" and "magnitude()"? How about choosing just one? I suggest "magnitude". > + void setMagnitudePhase(double mag, double phase) > + { > + m_real = mag * cos(phase); > + m_imag = mag * sin(phase); > + } It seems like we'd want this to be a function that returns a Complex, rather than a setter. > +protected: > + double m_real; > + double m_imag; private, not protected.

Created attachment 48065 [details] addresses Darin Adler's initial comments Hi Darin, thanks for having such a quick look!

(In reply to comment #3) > Hi Sam, I anticipate extending this class in the future for various DSP > algorithms, so I'd prefer to use our own implementation. I think we can add lots of functions that work with std::complex too, though, so I’m not sure a new class is the right way to go.

subclassing std::complex to add additional methods looks like a problem since the ivars are private. I'd prefer, if at all possible, to have something lightweight and more easily modifiable to our needs.

(In reply to comment #7) > subclassing std::complex to add additional methods looks like a problem since > the ivars are private. I'd prefer, if at all possible, to have something > lightweight and more easily modifiable to our needs. The technique is not subclassing. It's functions that take and return complex objects. I hear you expressing your preference. But I don’t see a good reason to consider std::complex heavyweight. And it comes with many of the functions you’d want to use already implemented. We should really consider it seriously before adding our own. Maybe you could show some coding examples to clarify why the code ends up looking so much cleaner with your class than if you used std::complex?

OK - don't want to rock the boat too much :) I'm sure it will be possible to adapt to std::complex. This will probably be fine for the current engine code. If it starts to get too messy, as I've been fearing, then we can revisit it later. Thanks for having a look - I guess we can close out this bug...

Closing as wontfix per Chris' comment above.

Comment on attachment 48065 [details] addresses Darin Adler's initial comments Clearing review flag per Chris's above comment.

Sorry, I'm re-opening this. I posted to webkit-dev with the problems with std::complex - re-pasting here: In the process of switching my code over to use std::complex I noticed a conflict with isinf(), isnan(), etc. The problem is that simply including: #include <complex> breaks the isinf(), isnan() functions (and some others I think). So now I'm getting compile errors in any header files which use these functions, such as WebGLFloatArray.h (which I need to include for music visualizer stuff). I'm a bit queasy about all the side-effects of simply including <complex> and am not even sure how to address the current situation, short of switching all of webkit over to using std::isinf, std::isnan, etc. Now I remember having similar problems with this in other codebases I've worked on, as the effects of <complex> seem to be viral... Unless somebody has a reasonable fix for the sinister side-effects of including <complex> I'd like my patch to be re-considered. Thanks, Chris

(In reply to comment #12) > So now I'm > getting compile errors in any header files which use these functions, such as > WebGLFloatArray.h Lets start by fixing those -- if it really ends up being too much of a pain we can fall back. I don’t want to give up at the first sign of trouble. It’s great to not have to reinvent the C++ library.

Hi Darin, I've been trying to move forward by changing all the uses of isnan() in the WebGL header files to std::isnan(). This also required including <cmath> in all of these header files so that std::isnan() is defined. But now this appears to be having a cascading effect, and this creates a compile error in JSImmediate.h (a use of the signbit() function) in JavaScriptCore. After including <cmath> here and using std::signbit(), I'm now getting a compile error in CachedResource ( function isfinite() ). I've found that the isfinite() function is used many places in the codebase, as well as isinf(), isnan(), etc. etc. So, I'm not sure what to do. I'm very uncomfortable trying to do a massive munge to convert the whole codebase to be <cmath> friendly since I'm very new here, especially considering possible differences across platforms. I think it would be a very large number of changes. What do you think we should do?

(In reply to comment #14) > Hi Darin, I've been trying to move forward by changing all the uses of isnan() > in the WebGL header files to std::isnan(). This also required including > <cmath> in all of these header files so that std::isnan() is defined. Could you post your patch in progress somewhere so I can spend a little time on this?

One of the first things I noticed is that many of those isnan checks are unneeded, because converting NaN to an integer already yields a zero. Not directly relevant, but something I spotted while looking at this.

Created attachment 48516 [details] Patch

Comment on attachment 48516 [details] Patch I've removed my original patch where I created a new complex number class which was defined in Complex.h. Instead, I've added a utilities header for adding new methods on std::complex as recommended by Darin Adler.

Comment on attachment 48516 [details] Patch I like everything about this except for the suffix Utilities. I think this file would be just fine named Complex.h -- named after the typedef that's in it. > +#include <stdio.h> Why include this? We should not. I'm going to say review- because I think we should not choose this filename.

Created attachment 48533 [details] Patch

Comment on attachment 48533 [details] Patch changed filename from ComplexUtilities.h to Complex.h also removed #include <stdio.h>

Comment on attachment 48533 [details] Patch Clearing flags on attachment: 48533 Committed r54684: <http://trac.webkit.org/changeset/54684>

All reviewed patches have been landed. Closing bug.