Sitelet https://web.archive.org/web/20201207081612/https://github.com/cpputest/cpputest/issues/1413
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy all buffers in `expectOneCall()` so that you don't have to keep them alive yourself? #1413

Open
evg-zhabotinsky opened this issue Aug 7, 2020 · 0 comments

Comments

@evg-zhabotinsky
Copy link

@evg-zhabotinsky evg-zhabotinsky commented Aug 7, 2020 •

Consider the following code:

void ExpectMatrix(int w, int h) {
  for (int i = 0; i < w; i++)
    for (int j = 0; i < h; j++)
      mock().expectOneCall("GetTwoInts")
        .withOutputParameterReturning("first", i, sizeof(int))
        .withOutputParameterReturning("second", j, sizeof(int));
}

As it is now, all the matching actualCall().withOutputParameter()s will read the same memory that will contain garbage, that is, where the local variables i and j used to be before the function returned.

Is there a painless way to make it work properly? If so, I'm unaware of it.
One could new() the temporary storage before every expectOneCall() and then delete() it after actualCall(), yes. But this is painful to look at, especially when you don't do that for withIntParameter() and similar. And if I'm not mistaken it can lead to memory leaks if the actualCall() never happens. At least that's how I see the cleanest possible version of it:

struct GetTwoIntsReturns {
  int first;
  int second;
}
void ExpectMatrix(int w, int h) {
  for (int i = 0; i < w; i++)
    for (int j = 0; i < h; j++)
      mock().expectOneCall("GetTwoInts")
        .andReturnValue(new GetTwoIntsReturns{i, j});
}
void GetTwoInts(int* first, int* second) {
  auto r = static_cast<GetTwoIntsReturns*>(
    mock().actualCall("GetTwoInts").returnPointerValue());
  *first = r->first;
  *second = r->second;
  delete r;
}

As compared to when the parameters aren't output:

void ExpectMatrix(int w, int h) {
  for (int i = 0; i < w; i++)
    for (int j = 0; i < h; j++)
      mock().expectOneCall("GetTwoInts")
        .withIntParameter("first", i)
        .withIntParameter("second", j);
}
void GetTwoInts(int first, int second) {
  mock().actualCall("GetTwoInts")
        .withIntParameter("first", first)
        .withIntParameter("second", second);
}

As another note, it is highly unobvious that you have to keep the buffers alive yourself. And I don't think docs say that anywhere, too. I personally spent quite some time debugging my first set of tests using output parameters exactly due to that, and also found a couple places in our code base where others made the same mistake but it didn't blow up.

P.S. Does withMemoryBufferParameter() copy its data? What about withParameterOfType()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.