c++ error using std::accumulate creating Least Common Multiple class

Question

I am trying to write a class the calculates the Least Common Multiple:

LeastCommonMultiple.h:

#pragma once

#ifndef LEASTCOMMONMULTIPLE_H
#define LEASTCOMMONMULTIPLE_H

#include <vector>
#include <numeric>

class LeastCommonMultiple
{
public:
    LeastCommonMultiple(std::vector<int>&);

    int GetLcmForVector();

private:
    std::vector<int> &m_list;

    int GetGreatestCommonDivisor(int, int);
    int GetLeastCommonMultiple(int, int);
};
#endif //! LEASTCOMMONMULTIPLE_H

LeastCommonMultiple.cpp

#include "stdafx.h"
#include "LeastCommonMultiple.h"

LeastCommonMultiple::LeastCommonMultiple(std::vector<int>& list) : m_list(list) 
{}

int LeastCommonMultiple::GetGreatestCommonDivisor(int a, int b)
{
    for (;;)
    {
        if (a == 0)
        {
            return b;
        }           

        b %= a;

        if (b == 0) 
        {
            return a;
        }

        a %= b;
    }
}

int LeastCommonMultiple::GetLeastCommonMultiple(int a, int b)
{
    int temp = LeastCommonMultiple::GetGreatestCommonDivisor(a, b);

    return temp ? (a / temp * b) : 0;
}

int LeastCommonMultiple::GetLcmForVector()
{
//std::accumulate takes first, last, initial value, operation
    int result = std::accumulate(m_list.begin(), m_list.end(), 1, LeastCommonMultiple::GetLeastCommonMultiple);

    return result;
}

I am getting the error:

Error C3867 'LeastCommonMultiple::GetLeastCommonMultiple': non-standard syntax; use '&' to create a pointer to member myproject

Before making this code object Oriented I tested it using the GetLcmForVector() function in main() and passing std::accumulate an array and it worked fine.

I have read other questions on this error and the issue was forgetting to add parenthesis at the end of the function BUT I should not have to do that with LeastCommonMultiple::GetLeastCommonMultiple since it is being used in std::accumulate

I am basing my code off this post:

C++ algorithm to calculate least common multiple for multiple numbers

is this really the error you get? There is no `EarliestDeadlineFirstScheduling` in the code — 463035818_is_not_an_ai, Feb 02 '18 at 13:18
Sorry, I should have taken that part out of the error. This piece of code is from a bigger project that I am working on. — JoeG, Feb 02 '18 at 13:53

Jarod42 · Answer 1 · 2018-02-02T13:14:01.650

3

As state in error message: missing &:

std::accumulate(m_list.begin(),
                m_list.end(),
                1,
                &LeastCommonMultiple::GetLeastCommonMultiple);

In addition the method should be static.

Regular free functions don't requires & due to heritage of C :/

edited Feb 02 '18 at 13:14

answered Feb 02 '18 at 13:01

Jarod42

203,559
14
181
302

I'd make `GetLeastCommonMultiple` a free function rather than a static, and also make `LeastCommonMultiple` a namespace – Caleth Feb 02 '18 at 13:28
I didn't realize I had to pass the function as reference to std::accumulate, thank you for clarifying the actual error message for me. – JoeG Feb 02 '18 at 14:21

Arnav Borborah · Accepted Answer · 2018-02-02T13:13:49.403

I would use std::bind for this:

int result = std::accumulate(m_list.begin(), m_list.end(), 1, 
             std::bind(
                 &LeastCommonMultiple::GetLeastCommonMultiple, 
                 this, 
                 std::placeholders::_1, 
                 std::placeholders::_2
             ));

This "binds" the member function you are trying to use with the current object. (The std::placeholder::_X represents one of the parameters of the function you are trying to pass)

On the other hand, you could make the method you are trying to use static and use & when passing it to std::accumulate.

Finally, you could even use a lambda here! _{Thanks to @Jarod42 for this}

int result = std::accumulate(m_list.begin(), m_list.end(), 1, 
            [this](int acc, int v) 
            { 
                return GetLeastCommonMultiple(acc, v); 
            });

On a side note, for this line of code:

int temp = LeastCommonMultiple::GetGreatestCommonDivisor(a, b);

You don't need the class name mentioned with the member function since it is not static.

Thank you for this complete answer and introducing me to std::bind, worked perfect! — JoeG, Feb 02 '18 at 14:14

score 1 · Answer 3 · answered Feb 02 '18 at 13:09

You could make LeastCommonMultiple a static method since it uses no state.

    int static GetLeastCommonMultiple(int a, int b);
    int static GetGreatestCommonDivisor(int a, int b);

...

    int result = std::accumulate(m_list.begin(), m_list.end(), 1, 
                                 &LeastCommonMultiple::GetLeastCommonMultiple);

c++ error using std::accumulate creating Least Common Multiple class

3 Answers3