Project

General

Profile

Actions

Bug #1358

closed

round_str in WebUtils.c dysfunctional for larger than 6 digits

Added by Samuel - over 12 years ago. Updated about 12 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Start date:
07/16/2012
Due date:
% Done:

0%

Estimated time:
1:00 h

Description

This function

char *round_str(double d, int digits, char *buf) {
  static constint exp[] = { 1, 10, 100, 1000, 10000, 100000, 1000000 };

  long long i = static_cast<longlong>(d * exp[digits] + (d > 0 ? 0.49 : -0.49));
  lltoa(i, buf);
  char *num = buf;

  if (num[0] == '-')
    ++num;
  int len = std::strlen(num);

  if (len <= digits) {
    int shift = digits + 1 - len;
    for (int i = digits + 1; i >= 0; --i) {
      if (i >= shift)
       num[i] = num[i - shift];
      else
       num[i] = '0';
    }
    len = digits + 1;
  }
  int dotPos = (std::max)(len - digits, 0);
  for (int i = digits + 1; i >= 0; --i)
    num[dotPos + i + 1] = num[dotPos + i];
  num[dotPos] = '.';

  return buf;
}

does not work if there are more than 6 digits requested. The broken lines are

  static constint exp[] = { 1, 10, 100, 1000, 10000, 100000, 1000000 };

  long long i = static_cast<longlong>(d * exp[digits] + (d > 0 ? 0.49 : -0.49));

For some reason, there is a hard coded array of powers of ten. If a programmer passes, say, 7 as digits, this won't work. It wasn't segfaulting or anything, it was just giving out totally garbage data. Ideally this would just be something like

long long i = static_cast<longlong>(d * pow(10, digits) + (d > 0 ? 0.49 : -0.49));

If the array was being used for speed purposes, at least do something like

static constint exp[] = { 1, 10, 100, 1000, 10000, 100000, 1000000 };

long long i = static_cast<longlong>(d * (digits <= 6 ? exp[digits] : pow(10, digits)) + (d > 0 ? 0.49 : -0.49));

Files

floatrendering.patch (3.67 KB) floatrendering.patch Wim Dumon, 07/18/2012 11:39 AM
Actions #1

Updated by Wim Dumon over 12 years ago

Hello,

  • round_str() is internal API. How did you manage to pass '7' as parameter to round_str()? We carefully only use it with numbers <= 6.
  • we've had enough complaints on round_str()s behavior by now to do something about it.

I propose to apply the patch attached hereto. I have done a few comparisons (MSVS 2010), and concluded that:

1. old round_str() is about 2 times faster than boost.karma

  1. boost.karma is about 2 times faster than sprintf
  2. old round_str() does not properly handle special float values (nan, infinity)
  3. floats will always be rendered with proper precision when using boost.karma

remaining questions:

  • do we always generate correct floats with the new implementation?
  • does the performance of the extra test in round_str() matter?
  • is the performance of boost.karma acceptable when compiler is used without optimization?

There's other people looking at the issue for rendering very large amounts of floats without conversing them to strings, primarily for WebGL purposes.

Can you test the patch applied hereto? I'll put it in git if you don't find problems with it.

Wim

Actions #2

Updated by Samuel - over 12 years ago

I'm using a WDoubleSpinBox. setValue(double value) calls textFromValue() which then calls round_str(double d, int digits, char *buf). I had been trying different numbers of decimals to display on my WDoubleSpinBox via setDecimals(int precision), which allows decimals higher than 6.

I wasn't able to test the patch, but it looks good to me.

Actions #3

Updated by Wim Dumon over 12 years ago

Samuel,

That's the root cause of the problem - round_str() cannot be used there. The correct patch for that problem is below, I'll make sure it gets into the public git.

WString WDoubleSpinBox::textFromValue() const
{
#ifndef WT_TARGET_JAVA
  std::stringstream ss;
  ss.precision(precision_);
  ss << std::fixed << std::showpoint << value_;
  std::string result = ss.str();
#else
  char buf[30];

  std::string result = Utils::round_str(value_, precision_, buf);
#endif // WT_TARGET_JAVA
Actions #4

Updated by Koen Deforche over 12 years ago

  • Status changed from New to Resolved
  • Assignee set to Koen Deforche
  • Target version set to 3.2.3
Actions #5

Updated by Koen Deforche about 12 years ago

  • Status changed from Resolved to Closed

Fixed in Wt 3.2.3 RC1.

Actions

Also available in: Atom PDF