smart menu

[globaltourist]

So the few questions are:
-How would you advise to make the access specifier of print private and not public ?

The rest of the questions focus on a really specific function that i pinpointed that it causes all of the problems with adding submenus:

void CompositeMenu::addChild(ContainerComponent& child) //taking const reference causes c2440 error on line 31
{
	if (std::find_if(m_children.begin(),
					 m_children.end(),
					 [&child](const ContainerComponent* cmp)
					 {return cmp->getName() == child.getName(); }) == m_children.end()) //is it possible to still compare function addresses ?
	{
		if (std::is_same<decltype(child), CompositeMenu>::value)
		{
			m_children.emplace_back(&Action(child.getName(), std::bind(&ContainerComponent::call, &child))); 
		}//end of if
		else m_children.emplace_back(&child);
		if (m_enabledBoxedMenu) updateLengths(child.getName(), boxedLengthPadding);
		else                    updateLengths(child.getName(), bareLengthPadding);
	}//end of if
	else std::cout << "An Action with this name already exists.\n";
}

-How should i get past the c2440 error and still pass by const reference ?
-From what iv'e read i cant get a member function address that has the "this" pointer but i still want to be able to check if an Action with the same m_action exists is it possible ?
-m_children.emplace_back(&Action(child.getName(), std::bind(&ContainerComponent::call, &child))); In my opinion is the root of the problem, i know its an awful idea to pass a reference to a temporary object but i cant seem to find a way around, i tried using unique_ptr and shared_ptr but they cause even more errors which are thrown not from the code itself but the algorithm library, using new isn't even an option as its not guaranteed 100% to allocate memory and there is no error handling and its prone to cause memory leaks if i wont make a proper cleanup.
So the code line above is responsible for storing the call function of a CompositeMenu and its name within an Action object, and the use of std::bind is needed because of the type of m_action the problem is that when i run the code it uses CompositeMenu's print instead of Action's print although i stored it as an Action object, is it undefined behavior because of the temporary object ? how should i fix this issue ?

the source.cpp i used for testing:

#include"Menu.h"

void a() { std::cout << 'a'; }
void b() { std::cout << 'b'; }
void c() { std::cout << 'c'; }
void d() { std::cout << 'd'; }

int main()
{
	CompositeMenu menu;
	CompositeMenu submenu;
	submenu.addHeader("Sub menu");
	menu.addHeader("Main menu");
	menu.enableBoxedOptions();
	Action optA("a", a);
	Action optB("b", b);
	Action optC("c", c);
	Action optD("d", d);
	menu.addChild(optA);
	menu.addChild(optB);
	menu.addChild(optC);
	menu.addChild(optD);
    menu.removeChild(optB);
	/*
	submenu.addChild(optA);
	submenu.addChild(optB);
	submenu.addChild(optC);
	submenu.addChild(optD);
	*/
	menu.addChild(submenu);
	menu.call();
	return 0;
}

NOTE: Everything but the addChild method works perfectly and only adding a submenu causes the issue.

[TheIdeasMan]

Hi,

Well the good news is that I now have a job :+D The bad news is that I won't have a lot of time to make replies, or whether or not I will have internet access. However it will be a weeks on weeks off roster type of job.

I haven't had much time to look at your latest code, I have been getting ready for the job.

One probably irrelevant thing: Put your classes into files of their own, 1 header and 1 cpp for each class. Ideally put all your code into a namespace.

I am sure there are plenty of other people who are willing to help.

[globaltourist]

Hello,

That's great news ! Good luck on your new job !

I am sure there are plenty of other people who are willing to help.

I will post on more forums, i think i will find them :)

[coder777]

How should i get past the c2440 error and still pass by const reference ?In this context you can't. Considering the std::bind(...) the object passed must exist as long as it can be called from m_children. Thus it can be neither const nor temporary.

Change line 10:

m_children.emplace_back(new Action(child.getName(), std::bind(&ContainerComponent::call, &child)));

or better yet:

std::vector<std::shared_ptr<ContainerComponent> > m_children;
...
m_children.push_back(std::make_shared<Action>(child.getName(), std::bind(&ContainerComponent::call, &child)));

otherwise it will crash.

Note that std::bind(&ContainerComponent::call, &child) does not allow const (except for ... call() const, which probably doesn't make sense).

i tried using unique_ptr and shared_ptr but they cause even more errors

Use unique_ptr if there is only one owner.
Use shared_ptr if there are multiple owner of that object.

I strongly recommend that you learn to use smart pointer. Otherwise this will remain error prone.

Further more i would recommend to use the signal/slot mechanism from boost:

http://www.boost.org/doc/libs/1_64_0/doc/html/signals2.html

[globaltourist]

i did try using shared_ptr but it crashes the program i'l implement it again and paste the error here.

note that i would have to use std::shared_ptr<ContainerComponent> in all of the std::remove_if and std::find_if because the lambdas in them currently take a const ContainerComponent*

otherwise it will crash.

Strangely when i pass in &Action(child.getName(), std::bind(&ContainerComponent::call, &child)) it doesn't crash, the only problem it makes is that it uses CompositeMenu's print function instead of Action's print function this behavior also occurs when i change it to new Action(child.getName(), std::bind(&ContainerComponent::call, &child));.

just tested it with shared_ptr and it gives me an

expression :_CrtlsValidHeapPointer(block)

in

 file: minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp Line:888

and just crashes the program

[coder777]

Strangely when i pass in &Action(child.getName(), std::bind(&ContainerComponent::call, &child)) it doesn't crash

This is called undefined behavior. It is plain wrong hence you cannot do it.

smart pointer of course work. If it crashes you are doing something wrong. For example you shall not use shared_ptr with references (&).

Your design looks a bit overcomplicated. How about this:

class Menu
{
...
  std::string m_Name;
  std::string m_Text;
  std::function<void()> m_CallFunction; // This might be a vector if you want to allow muliple calls
  std::vector<std::shared_ptr<Menu> > m_Children;
...
  std::shared_ptr<Menu> find_by_name(const std::string &name) // If the top level menu has just children this works
  {
    for(const std::shared_ptr<Menu> child : m_Children)
    {
      if(child->m_Name == name)
        return child;
      else
      {
        std::shared_ptr<Menu> m = child->find_by_name(name);
        if(m)
          return m;
      }
    }
    return std::shared_ptr<Menu>();
  }
};

[globaltourist]

Strangely when i pass in &Action(child.getName(), std::bind(&ContainerComponent::call, &child)) it doesn't crash
This is called undefined behavior. It is plain wrong hence you cannot do it.

smart pointer of course work. If it crashes you are doing something wrong. For example you shall not use shared_ptr with references (&).

After some testing i pinpointed what caused the crashes and it was m_children.emplace_back(&child) as it's firstly a reference to child and if m_children is a shared_ptr i cant store it but other than that what i found out is that:
if (std::is_same<decltype(child), CompositeMenu>::value) is never true and never gets executed thus the strange behavior is from m_children.emplace_back(&child) (when not using std::shared_ptr) my guess is that it cant deduce to the most derived class and should perhaps be tested with dynamic_cast.

class Menu
{
...
  std::string m_Name;
  std::string m_Text;
  std::function<void()> m_CallFunction; // This might be a vector if you want to allow muliple calls
  std::vector<std::shared_ptr<Menu> > m_Children;
...
  std::shared_ptr<Menu> find_by_name(const std::string &name) // If the top level menu has just children this works
  {
    for(const std::shared_ptr<Menu> child : m_Children)
    {
      if(child->m_Name == name)
        return child;
      else
      {
        std::shared_ptr<Menu> m = child->find_by_name(name);
        if(m)
          return m;
      }
    }
    return std::shared_ptr<Menu>();
  }
};

I cant understand what this should replace and what the purpose of this piece of code is, can you please explain ?

[coder777]

After some testing i pinpointed what caused the crashes and it was m_children.emplace_back(&child) as it's firstly a reference to child and if m_children is a shared_ptr i cant store it but other than that what i found out is that:

So, it is always problematic to store a raw pointer where the life span of this object is unknown.
It crashen because shared_ptr takes ownership of the pointer. It actually deletes the object.

if (std::is_same<decltype(child), CompositeMenu>::value) is never true

Yes, because the type of child is ContainerComponent.

I cant understand what this should replace

Well, this is the design idea without CompositeMenu or Action. The intention is to use it like so:

#include"Menu.h"

void a() { std::cout << 'a'; }
void b() { std::cout << 'b'; }
void c() { std::cout << 'c'; }
void d() { std::cout << 'd'; }

int main()
{
  Menu m{"Welcome message"};
  m.m_Children.push_back(std::make_shared<Menu>("Menu a", a));
  m.m_Children.push_back(std::make_shared<Menu>("Menu b", b));
  m.m_Children.push_back(std::make_shared<Menu>("Menu c", c));
  m.run();

	return 0;
}

[globaltourist]

Well, this is the design idea without CompositeMenu or Action. The intention is to use it like so:

I actually posted a code without any pattern in here and the point was to remake it with the composite design pattern as it makes more sense, its more flexible, and if i want to add other containers such as a composite directory to the code i can easily do it .

Finally i did make it work and simplified it a little myself here is the link to the final code:
https://codereview.stackexchange.com/questions/166792/menu-that-utilizes-the-composite-design-pattern

[coder777]

the point was to remake it with the composite design pattern

Okay, I guess I missed that point.

is it possible to still compare function addresses ?

To answer this question: Yes you can compare std::function objects.

[globaltourist]

To answer this question: Yes you can compare std::function objects.

Unfortunately i cant make it work, from what iv'e read they are wrappers and you cant compare wrappers. Getting the address from the wrapper is also out of question, the functions compared are member function and have the thisptr the address of which is not accessible.
Any suggestions ?

[coder777]

I see that you can't use the operator== directly (except for nullptr). What you can actually do is compare target_type():

http://www.cplusplus.com/reference/functional/function/target_type/

[globaltourist]

The problem with that is that all of the functions are of type void() and it would always return true

[coder777]

target_type actually returns the whole function name. Admittedly not so relyable for c functions. But you can retrieve the address of the function:

http://www.cplusplus.com/reference/functional/function/target/

[globaltourist]

Well in theory it does but the function is a member function and it is not possible to get member function addresses without making it very messy.