Hardening/2603 int destruction

[kzangeli]

Fixed issue telefonicaid#2603 and part of telefonicaid#2425
CNR missing ...

[fgalan]

Instead of defining a new function, this algorithm can be included in the current toString(double) function. I mean:

template <> std::string toString(double f)
{
  char  buf[STRING_SIZE_FOR_DOUBLE];
  int bufSize = sizeof(buf);

  long long  intPart   = (long long) f;
  double     diff      = f - intPart;
  bool       isInteger = false;

  // abs value for 'diff'
  diff = (diff < 0)? -diff : diff;

  if (diff > 0.9999999998)
  {
    intPart += 1;
    isInteger = true;
  }
  else if (diff < 0.000000001)  // it is considered an integer
  {
    isInteger = true;
  }

  if (isInteger)
  {
    snprintf(buf, bufSize, "%lld", intPart);
  }
  else
  {
    snprintf(buf, bufSize, "%.9f", f);

    // Clear out any unwanted trailing zeroes
    char* last = &buf[strlen(buf) - 1];

    while ((*last == '0') && (last != buf))
    {
      *last = 0;
      --last;
    }
  }

  return std::string(buf);
}

This way no additional modifications in other places of the code are required as the calling interface for toString(double) doesn't change.

[kzangeli]

Let's try to avoid unnecessarily slow code, and try to avoid returning objects on the stack.

About changing the calls to toString, those changes are already made.
There is absolutely nothing to win in changing the function from returning a std::string instead of the char* it is returning right now. That would only give more work and slower code.

An additional advantage with the current implementation - it can be used without assignment on the caller side:

char buf[xxx];
toString(dValue, buf, sizeof(buf));

NTC

[fgalan]

Let’s try to avoid unnecessary changes not directly related with the issue the PR is attempting to solve, especially when these changes increase the complexity of the code.

Complexity increase should be evident. For instance, the statement

return toString(numberValue);

is much simpler than

char numberBuffer[STRING_SIZE_FOR_DOUBLE];
return toString(numberValue, numberBuffer, sizeof(numberBuffer));

The need of to pre-declare an extra variable increases complexity. Change function call signature from 1 to 3 parameters increase complexity. Having the same parameter (numberBuffer) at the same time as input parameter and return parameter increases complexity. And multiply this increase of complexity for all places in which the function is being called. Keeping complexity low is a win.

The proposed solution (i.e. apply the rounding algorithm in the toString() function but without changing its signature) doesn’t give slower code. The code is not slower than the one currently in master. In fact, it is probably faster (as std::ostringstream is no longer used). The size of std::string in the stack is not a problem (as, as we measure, the size is exact the same than the one of a char* for the reference Orion sytem, i.e. RHEL/CentOS 7 and, even in other system, the increase is not significant).

This PR should stick to the issue is attempting to solve and not trying to change a function pattern in a way it has not been previously agreed. Before implementing this kind of changes we should have the proper discussion (maybe the right place to have it is telefonicaid#1298).

[kzangeli]

There is really not much to discuss here, but sure, we can create an issue and discuss it there.
But right now we have more urgent matters.
This PR fixes a pretty ugly bug that destroys big numeric values and people are waiting for it to be fixed.
We need to merge this PR ASAP.
After that we have all the time in the world to discuss this matter.

So again,

NTC

[kzangeli]

Already merged info the "main fork"