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
– 463035818_is_not_an_ai, Commented Feb 2, 2018 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
– JoeG, Commented Feb 2, 2018 at 13:53

Arnav Borborah · Accepted Answer · 2018-02-02 13:13:49Z

3

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.

edited Feb 2, 2018 at 13:13

answered Feb 2, 2018 at 13:04

Arnav Borborah

11.8k8 gold badges51 silver badges94 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

JoeG Over a year ago

Thank you for this complete answer and introducing me to std::bind, worked perfect!

Jarod42 · Accepted Answer · 2018-02-02 13:14:01Z

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 2, 2018 at 13:14

answered Feb 2, 2018 at 13:01

Jarod42

227k15 gold badges214 silver badges350 bronze badges

2 Comments

Caleth Over a year ago

I'd make GetLeastCommonMultiple a free function rather than a static, and also make LeastCommonMultiple a namespace

JoeG Over a year ago

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.

Richard Hodges · Accepted Answer · 2018-02-02 13:09:12Z

1

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);

answered Feb 2, 2018 at 13:09

Richard Hodges

70.3k8 gold badges103 silver badges157 bronze badges

Collectives™ on Stack Overflow

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

3 Answers 3

1 Comment

2 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related