Is there an issue with c string pointer arithmetic in Arduino?

CMyString str;
str.format(3, "m_uNumAngularSlices = %d, m_uRotationPeriod = %ld, uRadiusClock = %lX", (int)m_uNumAngularSlices, (int)m_uRotationPeriod, (int)uRadiusClock);

If I call the format(...) function like this then:

The first call to getNextFormatType(...) will return
lpszFormatStr = ", m_uRotationPeriod = %ld, m_uRadiusClock = %lX"
rStrText = "m_uNumAngularSlices = "
rType = "%d"

The second call to getNextFormatType(...) will return
lpszFormatStr = ", m_uRadiusClock = %lX"
rStrText = ", m_uRotationPeriod = "
rType = "%ld"

And so on.

"lpszFormatStr = lpszEndType;"

This line of code seems to be the issue.

I can Serial.print(lpszEndType) and I get the expected string output in serial monitor.

But as soon as I try to return the string to the previous function in any way, shape or form it stuffs the program up and I get garbage in serial monitor.

I have tried this way with a reference parameter (char *).
I have tried with getNextFormatType returning the new pointer
I have tried by assigning the substring pointed to by lpszEndType to a CMyStringObject and returning the reduced format string as a reference parameter and a function return value.

All with the same result - if I try to return it to the previous function I get garbage in serial monitor.
If I comment out "lpszFormatStr = lpszEndType;" then I get sensible output in serial monitor.
If I comment out my previous line of rFormatStr = lpszEndType (where rFormatStr is a CMyString & aparameter )then I get sensible output in serial monitor.
If I uncomment rFormatStr = lpszEndType then I get garbage in serial monitor.

I am at a total loss. Why can't I do this seemingly simple operation?

void CMyString::getNextFormatType(char *&lpszFormatStr, CMyString &rStrText, CMyString &rType)
{
  static char *lpszSartType = 0, *lpszEndType = 0;
  int nTypeLen = 0, nPos = 0;

  lpszSartType = strchr(lpszFormatStr, '%');
  if (!(lpszEndType = strchr(lpszFormatStr, 'd')))
  {
    if (!(lpszEndType = strchr(lpszFormatStr, 'i')))
    {
      if (!(lpszEndType = strchr(lpszFormatStr, 'o')))
      {
        if (!(lpszEndType = strchr(lpszFormatStr, 'x')))
        {
          if (!(lpszEndType = strchr(lpszFormatStr, 'X')))
          {
            if (!(lpszEndType = strchr(lpszFormatStr, 'F')))
            {
              if (!(lpszEndType = strchr(lpszFormatStr, 'f')))
              {
                if (!(lpszEndType = strchr(lpszFormatStr, 'G')))
                {
                  if (!(lpszEndType = strchr(lpszFormatStr, 'g')))
                  {
                    if (!(lpszEndType = strchr(lpszFormatStr, 'E')))
                    {
                      if (!(lpszEndType = strchr(lpszFormatStr, 'e')))
                      {
                        if (!(lpszEndType = strchr(lpszFormatStr, 'A')))
                        {
                          if (!(lpszEndType = strchr(lpszFormatStr, 'a')))
                          {
                            if (!(lpszEndType = strchr(lpszFormatStr, 'c')))
                            {
                              if (!(lpszEndType = strchr(lpszFormatStr, 's')))
                              {
                                if (!(lpszEndType = strchr(lpszFormatStr, 'p')))
                                {
                                }
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }
  if (lpszSartType)
  {
    nTypeLen = lpszEndType - lpszSartType + 1;
    strncpy(rType.getBuffer(), lpszSartType, nTypeLen);
    rType.getBuffer()[nTypeLen] = 0;
    nPos = lpszSartType - lpszFormatStr;
    strncpy(rStrText. getBuffer(), lpszFormatStr, nPos);
    lpszEndType++;
    lpszFormatStr = lpszEndType;
  }
}

CMyString &CMyString::format(const int nNumParams, char *lpszFormat, const int nNum, ...)
{
  int nI = 0, nPos = 0;
  CMyString strType, strTxt, str;
  strTemp.empty();
  va_list arguments;
  va_start(arguments, nNumParams);           

  for (nI = 0; nI < nNumParams; nI++)        
  {
      getNextFormatType(lpszFormat, strTxt, strType);
      
 Serial.print(lpszFormat);
 Serial.print(", ");
 Serial.print(strTxt);
 Serial.print(", ");
 Serial.println(strType);
 Serial.print(", ");
 Serial.println(nI);
 Serial.println();

      if (strType.getLength() > 0)
      {
        strTemp += strTxt;
        sprintf(str.getBuffer(), strType,  va_arg(arguments, int));
        strTemp += str;
      }
  }
  va_end(arguments);

  return strTemp;
}

You seem to be mixing Strings and strings.

In any case, strncpy() does not guarantee a zero terminated string.

Two suggestions:

  1. Use code tags. Mixing C and English together makes for a confusing read.

  2. This is evil:

  if (!(lpszEndType = strchr(lpszFormatStr, 'd')))
  {
    if (!(lpszEndType = strchr(lpszFormatStr, 'i')))
    {
      if (!(lpszEndType = strchr(lpszFormatStr, 'o')))
      {
        if (!(lpszEndType = strchr(lpszFormatStr, 'x')))
        {
          if (!(lpszEndType = strchr(lpszFormatStr, 'X')))
          {
            if (!(lpszEndType = strchr(lpszFormatStr, 'F')))
            {
              if (!(lpszEndType = strchr(lpszFormatStr, 'f')))
              {
                if (!(lpszEndType = strchr(lpszFormatStr, 'G')))
                {
                  if (!(lpszEndType = strchr(lpszFormatStr, 'g')))
                  {
                    if (!(lpszEndType = strchr(lpszFormatStr, 'E')))
                    {
                      if (!(lpszEndType = strchr(lpszFormatStr, 'e')))
                      {
                        if (!(lpszEndType = strchr(lpszFormatStr, 'A')))
                        {
                          if (!(lpszEndType = strchr(lpszFormatStr, 'a')))
                          {
                            if (!(lpszEndType = strchr(lpszFormatStr, 'c')))
                            {
                              if (!(lpszEndType = strchr(lpszFormatStr, 's')))
                              {
                                if (!(lpszEndType = strchr(lpszFormatStr, 'p')))
                                {
                                }
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }

I'm not entirely sure what you're trying to accomplish there, but man.. there's gotta be a better way! :slight_smile:

switch/case comes to mind.

and (I think) == not =

Mark

Also lose the Hungarian notation.

SirNickity:
Two suggestions:

  1. Use code tags. Mixing C and English together makes for a confusing read.

  2. This is evil:

  if (!(lpszEndType = strchr(lpszFormatStr, 'd')))

{
    if (!(lpszEndType = strchr(lpszFormatStr, 'i')))
    {
      if (!(lpszEndType = strchr(lpszFormatStr, 'o')))
      {
        if (!(lpszEndType = strchr(lpszFormatStr, 'x')))
        {
          if (!(lpszEndType = strchr(lpszFormatStr, 'X')))
          {
            if (!(lpszEndType = strchr(lpszFormatStr, 'F')))
            {
              if (!(lpszEndType = strchr(lpszFormatStr, 'f')))
              {
                if (!(lpszEndType = strchr(lpszFormatStr, 'G')))
                {
                  if (!(lpszEndType = strchr(lpszFormatStr, 'g')))
                  {
                    if (!(lpszEndType = strchr(lpszFormatStr, 'E')))
                    {
                      if (!(lpszEndType = strchr(lpszFormatStr, 'e')))
                      {
                        if (!(lpszEndType = strchr(lpszFormatStr, 'A')))
                        {
                          if (!(lpszEndType = strchr(lpszFormatStr, 'a')))
                          {
                            if (!(lpszEndType = strchr(lpszFormatStr, 'c')))
                            {
                              if (!(lpszEndType = strchr(lpszFormatStr, 's')))
                              {
                                if (!(lpszEndType = strchr(lpszFormatStr, 'p')))
                                {
                                }
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }




I'm not entirely sure what you're trying to accomplish there, but man.. there's gotta be a better way! :)

Well I am open to suggestions.

The point of the above was to stop more comparisons being done than is necessary.......execution time and all that.

KeithRB:
switch/case comes to mind.

I can't really see how I could use a switch/case in this particular situation.

holmes4:
and (I think) == not =

Mark

No, because I am testing lpszEndType for NULL - strchr(...) returns NULL if it fails to find the char of interest.

I could have done it as follows:

lpszEndType = strchr(lpszFormatStr, 'o');
if (!lpszEndType )
{
lpszEndType = strchr(lpszFormatStr, 'X');
if (!lpszEndType )
{
.....
}
}

But then that would have been worst again.

It would have been a lot more convenient if there was a string function along the lines of FindOneOf(lpszFormatStr, "idxXofF.....").

KeithRB:
Also lose the Hungarian notation.

Hungarian notation?

Note that without any elses, every single if statement will be executed. So much for efficiency!

You are correct, switch case would be awkward.

check out strcspn()

size_t idx = strcspn(FormatString, "dioxX...");
EndType = FormatString + idx;

It would have been a lot more convenient if there was a string function along the lines of FindOneOf(lpszFormatStr, "idxXofF.....").

Actually I just had another look through the string functions and there is a FindOneOf(...) type function - strpbrk(...)

So I can replace those ugly nested if statements with:

lpszEndType = strbrk(lpszFormatStr, "diucsfFgGhH");

I will have to re-arrange things a little so that it does not find any of "diucsfFgGhH" within the ordinary text preceding the %d or what ever.

KeithRB:
Note that without any elses, every single if statement will be executed. So much for efficiency!

You are correct, switch case would be awkward.

check out strcspn()

size_t idx = strcspn(FormatString, "dioxX...");

EndType = FormatString + idx;

OK. That would be better than strbrk because it will eliminate the need for pointer arithmetic by the look of it, since it returns an index effectively rather than a pointer to the occurrence.

So I can replace those ugly nested if statements with:

Or, you could put the characters to check for in an array, and use a for loop to iterate through the array. break; if you find a match.

Don't expect people to help you debug snippets. What the hell is a CMyString anyway?

No one outside of Microsoft uses names like lpszFormatStr. Why is the pointer long? Do we really care? pStr would tell us that the variable is a pointer to a string, and that's all we really need to know.

It would have been a lot more convenient if there was a string function along the lines of FindOneOf(lpszFormatStr, "idxXofF.....").

The beauty of C is that you can create your own function to do it if the library does not provide one.

In the spirit of trying to help:

OK, first, ... yeah, that Hungarian notation stuff is horrible. (lpsz, et all) You're free to use your own naming conventions of course, but the majority of the civilized world (which is... anywhere outside MS HQ) considers it cluttered and useless to prepend all those abbreviations to variable names. It's ugly, and no one but ex-VB programmers will ever want to look at it, but if it works for you, well.. so be it. Just expect to hear groans and sighs from the peanut gallery. :wink:

Next, you probably should give your readers a little bit of an introduction to what you're trying to accomplish (which is what I was asking earlier, to which you replied "execution speed and all that"). I don't think anyone here "gets" the purpose of CMyString yet. What's the point of this class? What does it do that you can't already do with sprintf() or whatever?

Finally, PaulS is right on the money. We're all severely handicapped since you haven't included the whole class, or even all the parts you use. For example, what does getBuffer() do? There are also zero comments. So, with no context, missing code, distracting naming conventions, and code snippets pasted right in the body text of your post, it takes a lot of effort to even guess at what's going wrong. The forum helps those that help the forum.... :wink:

PaulS:

So I can replace those ugly nested if statements with:

Or, you could put the characters to check for in an array, and use a for loop to iterate through the array. break; if you find a match.

Don't expect people to help you debug snippets. What the hell is a CMyString anyway?

No one outside of Microsoft uses names like lpszFormatStr. Why is the pointer long? Do we really care? pStr would tell us that the variable is a pointer to a string, and that's all we really need to know.

I am a former windows C++ programmer so old habits die hard.

I f I see lpsz 6 months later I know immediately what the variable or parameter is.

I thought some one might have some insight into why I am getting this problem with the pointer arithmetic because other arduino programmers have already run into the same problem.

because other arduino programmers have already run into the same problem.

Only those that expect us to guess at that the rest of the code looks like.

KeithRB:
Note that without any elses, every single if statement will be executed. So much for efficiency!

Kieth that can't be right.

If it went through all the if statements then, with the first iteration of the loop, lpszEndType would be NULL because there is no %p in my format string.

I.E. with a format string "m_uNumAngularSlices = %d, m_uRotationPeriod = %ld, uRadiusClock = %lX" my function would fail to find the first "%d". However I have already established that it does correctly find the first %d in the format string.

The problem has been that I can't re-set the format string to exclude the "m_uNumAngularSlices = %d" part without hanging the arduino.

[code]
  if (!(lpszEndType = strchr(lpszFormatStr, 'd')))
  {
    if (!(lpszEndType = strchr(lpszFormatStr, 'i')))
    {
      if (!(lpszEndType = strchr(lpszFormatStr, 'o')))
      {
        if (!(lpszEndType = strchr(lpszFormatStr, 'x')))
        {
          if (!(lpszEndType = strchr(lpszFormatStr, 'X')))
          {
            if (!(lpszEndType = strchr(lpszFormatStr, 'F')))
            {
              if (!(lpszEndType = strchr(lpszFormatStr, 'f')))
              {
                if (!(lpszEndType = strchr(lpszFormatStr, 'G')))
                {
                  if (!(lpszEndType = strchr(lpszFormatStr, 'g')))
                  {
                    if (!(lpszEndType = strchr(lpszFormatStr, 'E')))
                    {
                      if (!(lpszEndType = strchr(lpszFormatStr, 'e')))
                      {
                        if (!(lpszEndType = strchr(lpszFormatStr, 'A')))
                        {
                          if (!(lpszEndType = strchr(lpszFormatStr, 'a')))
                          {
                            if (!(lpszEndType = strchr(lpszFormatStr, 'c')))
                            {
                              if (!(lpszEndType = strchr(lpszFormatStr, 's')))
                              {
                                if (!(lpszEndType = strchr(lpszFormatStr, 'p')))
                                {
                                }


[/code]

Yeah you are correct, but you could do the same thing by including the elses and not have all the nested ifs.