0

Any ideas why the following code segfaults?

class foo represents any class. This seams like a good idea for providing an evented interface.

#include <iostream>
#include <functional>

class foo
{
public:
  foo()
  {
    val = 42;  
  };

  void bar(std::function<int(foo*)> f)
  {
    this->_bar = std::bind(f, this);
  }
  std::function<int()> _bar;
  int val;
};

int main(void)
{
  foo *foobar;
  foobar->bar([](foo *self)->int{return self->val;});

  std::cout << foobar->_bar();
}
nykac
  • 49
  • 3
  • 4
    Before using lambdas you should start to read a proper book, see the list of good books. http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list – Stephan Dollberg Dec 26 '12 at 11:00
  • Why do you cal it "evented"? And what is a "good idea" that you want to provide? – SChepurin Dec 26 '12 at 12:25
  • SO didn't let me post the code w/o 2 lines of desc. I found the bug as soon as I posted, but I couldn't delete the post. The good idea is using std::bind to give access to the current object. ie: pass the lamda that takes args self and then then some other thing, and binding the self argument to the this pointer. [entity.h](https://github.com/Tvorba/Tvorba-gfx/blob/master/entity.h) [entity.cpp](https://github.com/Tvorba/Tvorba-gfx/blob/master/entity.cpp) my main problem is coding when I am very sleep deprived. – nykac Dec 26 '12 at 22:27

4 Answers4

6

It segfauls because foobar is not pointing to anything, you should use foo foobar;.

EDIT:

A short review.

class foo
{
public:
  foo()
  {
    val = 42;  // Use initialization list.
  };

  void bar(std::function<int(foo*)> f) // Use reference (foo&) instead of pointer (foo*).
  {
    this->_bar = std::bind(f, this); // Move "f" into bind and use "std::ref()" so you can pass "*this" by reference without copying.
  }

  // Hide local member variables

  std::function<int()> _bar; // Never use _ as a prefix, only the compiler is allowed to use _ prefix.
  int val;
};

int main(void)
{
  foo *foobar; // Use value semantics, i.e. "foo foobar".

  foobar->bar([](foo *self)->int{return self->val;});

  std::cout << foobar->_bar();
}

e.g.

class foo
{
public:
    foo()
        : val_(42)
    {
    };

    void set_bar(std::function<int(foo&)> f)
    {
        bar_ = std::bind(std::move(f), std::ref(*this));
    }

    int invoke_bar() const
    {   
        return bar_;
    }

    int get_val() const
    {
        return val_;
    }

private:

    std::function<int()> bar_;
    int val_;
};

int main(void)
{
  foo foobar;
  foobar.set_bar([](foo& self) -> int
  {
     return self.get_val();
  });

  std::cout << foobar.invoke_bar();
}
ronag
  • 49,529
  • 25
  • 126
  • 221
  • 1
    No, he shouldn't. You just told someone to use `new` who obviously doesn't know any deeper concepts of dynamic memory, RAII etc. He is now probably going to leak and keep on using this pointer madness. – Stephan Dollberg Dec 26 '12 at 11:03
  • 1
    Names that begin with `_` are reserved only when underscore is followed by uppercase letter or for names in the global namespace. There is nothing wrong with using names starting with `_` followed by lowercase letter for class members. – Konstantin Oznobihin Dec 26 '12 at 11:52
  • "Underscores (`_') are often used in names of library functions (such as "_main" and "_exit"). In order to avoid collisions, do not begin an identifier with an underscore." From "Programming in C++, Rules and Recommendations"(http://www.doc.ic.ac.uk/lab/cplus/c++.rules/chap5.html). – SChepurin Dec 26 '12 at 11:59
  • i found the issue... but never clicked the submit button beofre a crash. – nykac Dec 26 '12 at 22:25
3

You never create a foo, you just declare a pointer to it. Change foo* foobar to foo foobar and then get the address to it with &. (Not that you need it in this example).

This should work I guess.

foo foobar;
foobar.bar([](foo* self){return self->val;});
std::cout << foobar._bar() << std::endl;
return 0;
Cubic
  • 14,902
  • 5
  • 47
  • 92
  • He could also use a reference for the lambda argument, instead of a pointer. – ronag Dec 26 '12 at 11:08
  • @ronag The way he uses it would then require him to dereference `this` in the call though. I thought this looked a bit nicer. – Cubic Dec 26 '12 at 11:38
2

Because you declared a foobar pointer but didn't use new to allocate memory for it. foo *foobar = new foo(); should make your code work. But you also use automatic storage duration and pass parameter to lambada by reference:

int main(void)
{
  foo foobar;
  foobar.bar([](foo *self)->int{return self->val;});

  std::cout << foobar._bar();
}
billz
  • 44,644
  • 9
  • 83
  • 100
  • No, he shouldn't. You just told someone to use `new` who obviously doesn't know any deeper concepts of dynamic memory, RAII etc. He is now probably going to leak and keep on using this pointer madness. – Stephan Dollberg Dec 26 '12 at 11:03
1

Since no one seems to have said it explicitly: your problem has nothing to do with lambda; you would have exactly the same problem with a normal functional object, or with an immediate function call. Before worrying about lambda and other "advanced" features, you must learn the basics of the language. In this case:

  1. You can never use an uninitialized variable, ever. The results of doing so with a pointer are generally more spectacular than when you use an uninitialized int, but it is undefined behavior in all cases. If a variable has class type, define constructors for it, which initialize all members (and base classes). If a variable doesn't have class type (and a pointer does not have class type), never define it without an initialization clause.

  2. C++ uses value semantics by default. Unless there is a good reason, you should not use pointers. Use a value.

Until you can write code which respects these principles without using bind and lambda, you shouldn't be looking at bind and lambda. You can't run until you know how to walk.

James Kanze
  • 150,581
  • 18
  • 184
  • 329