1

I have a function like so:

#include <stdio.h>
#include <opencv/cv.h>
#include <opencv/highgui.h>
#include <opencv/cxcore.h>
#include <math.h>

#define PI 3.14159265

#define GRADIENT_THRESHOLD 200.0
#define ANGLE_RANGE 20

using namespace cv;
using namespace std;

class Circle {
public:
    int x;
    int y;
    int r;

    Circle(int x, int y, int r) {
        this->x = x;
        this->y = y;
        this->r = r;
    }

    double area() {
        return PI * pow(r, 2);
    }
};



Vector <Circle> collect_circles_from_houghSpace(Mat &houghSpaceCircle, double voting_threshold) {
    int height = houghSpaceCircle.size[0];
    int width = houghSpaceCircle.size[1];
    int radius = houghSpaceCircle.size[2];
    std::vector <Circle> circles;
    for (int y = 0; y < height; y++) {
        for (int x = 0; x < width; x++) {
            for (int r = 0; r < radius; r++) {
                if (houghSpaceCircle.at<cv::Vec3i>(y, x)[r] > voting_threshold) {
                    circles.push_back(Circle(x, y, r));

                }
            }
        }
    }
    return circles;
}

When I compile it, I get an error saying: In file included from sobel.cpp:2:

In file included from /usr/local/Cellar/opencv@2/2.4.13.7_5/include/opencv/cv.h:64:
In file included from /usr/local/Cellar/opencv@2/2.4.13.7_5/include/opencv2/core/core.hpp:4932:
/usr/local/Cellar/opencv@2/2.4.13.7_5/include/opencv2/core/operations.hpp:2415:23: error: no matching constructor for initialization of 'Circle []'
        newData = new _Tp[newCapacity];
                      ^
/usr/local/Cellar/opencv@2/2.4.13.7_5/include/opencv2/core/operations.hpp:2400:13: note: in instantiation of member function 'cv::Vector<Circle>::reserve' requested here
            reserve(_size);
            ^
/usr/local/Cellar/opencv@2/2.4.13.7_5/include/opencv2/core/operations.hpp:2305:7: note: in instantiation of member function 'cv::Vector<Circle>::set' requested here
    { set(!vec.empty() ? (_Tp*)&vec[0] : 0, vec.size(), _copyData); }
      ^
sobel.cpp:166:12: note: in instantiation of member function 'cv::Vector<Circle>::Vector' requested here
    return circles;
           ^
sobel.cpp:15:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 0 were provided
class Circle {
      ^
sobel.cpp:21:5: note: candidate constructor not viable: requires 3 arguments, but 0 were provided
    Circle(int x, int y, int r) {
    ^
1 error generated.

Could someone explain the error to me and how to fix it? As I see it, I have created a constructor that takes in 3 arguments. I'm not sure I understand why the error is saying I have provided 0 args.

nz_21
  • 6,140
  • 7
  • 34
  • 80
  • @Miki hmm I see...that's weird. – nz_21 Nov 20 '19 at 14:18
  • @nz_21 It's not weird. Say you create std::vector vec(2); How will std::vector instantiate those 2 objects of type Circle? – mfnx Nov 20 '19 at 14:20
  • @Miki hmm...I'd rather exploit move semantics than have it copied over. I'm thinking of doing `return std::move(circles)` -- is that a good idea? – nz_21 Nov 20 '19 at 14:20
  • @nz_21, [no](https://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value)! – Evg Nov 20 '19 at 14:23
  • @nz_21 Please post a [mcve] (all C++ compiler errors should be able to be reproduced with very simple code). If all of that OpenCV stuff is removed, and just simple vector manipulations are left in, the issue [cannot be duplicated](https://coliru.stacked-crooked.com/a/f8db72fe06417df9) – PaulMcKenzie Nov 20 '19 at 14:25
  • @Evg ah thanks for the link...but how do I guarantee that it will be moved and not copied? – nz_21 Nov 20 '19 at 14:27
  • @nz_21, the C++ standard gives you that guarantee. BTW, you can write `circles.emplace_back(x, y, r);`. – Evg Nov 20 '19 at 14:28
  • @Evg gotcha, thanks! – nz_21 Nov 20 '19 at 14:37
  • 1
    Note that `cv::Vector` (your return type) is not the same as `std::vector` (the type of `circles`) . You probably want to stick to one of them. (`cv::Vector` does not exist in more recent OpenCV versions.) – molbdnilo Nov 20 '19 at 14:40

2 Answers2

2

The essential part of your function is:

cv::Vector<Circle> foo() {
    std::vector <Circle> circles;
    return circles;
}

Here the type of circles is not the return type. A type conversion should take place. cv::Vector<T> has an implicit constructor

Vector(const std::vector<T>& vec, bool copyData = false)

that will be called to perform the conversion.

std::vector<T> itself does not require T to be default constructible, so the error happens inside cv::Vector<T>::Vector. Its reserve() member function, which is called during construction, has the following line:

newData = new T[newCapacity];

This line does require T to be default constructible. That's why you get the error and that's why reserve() is mentioned in the error log. To fix it, declare a defaulted default constructor:

class Circle {
public:
    Circle() = default;
    ...
};

Also note, that data from circles will not be copied into Vector unless you set copyData to true. The return statement should look like this:

return {circles, true};

or

return cv::Vector<Circle>(circles, true);

With

return circles;

cv::Vector will store a pointer to &circles[0], that will become invalid after the local object circles is destroyed when the function returns.

A comment about std::move. You could try to do

return Vector<Circle>(std::move(circles), true);

This will not move anything. The shallow reason: the first constructor parameter has type const std::vector<T>& vec, you can't move from it, it's const. And the deep reason: to move from std::vector means to copy (or swap) an internal pointer to heap-allocated buffer. Implementation of std::vector is not specified, so there is no way to reuse its buffer in user-defined types like cv::Vector. cv::Vector allocates its own storage and copies elements into it, and you can't do better if you want cv::Vector to outlive std::vector.

The move semantics could have been useful if cv::Vector used std::vector internally to store its data and the constructor looked like this

Vector(std::vector<T> vec) : internal_vec(std::move(vec)) { }

To avoid unnecessary copies, you might want to use cv::Vector<Circle> circles; from the very beginning.

Evg
  • 25,259
  • 5
  • 41
  • 83
0
sobel.cpp:15:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 0 were provided
class Circle {

The error message suggest the copy constructor is missing. And this make sense because you are copying the Vector during return

nelson.l
  • 66
  • 2
  • this is one confusing language :( So, if I've got this right, the C++ standard guarantees it will be moved (not copied). And yet, I still have to create a copy constructor... – nz_21 Nov 20 '19 at 14:36
  • No, `Circle` does have an implicitly defined copy constructor. – Evg Nov 20 '19 at 14:37
  • 1
    It is because you are constructing a Vector from std::vector, and it is copying the elements. I am not familiar with Vector, but I guess you can try using `Vector circles` instead of `std::vector` for the loop – nelson.l Nov 20 '19 at 14:39
  • @Evg Agree. But Vector internally use Circle[] to store elements and this requires default constuctor – nelson.l Nov 20 '19 at 14:56