Iterating through QButtonGroup::buttons causes a crash

Question

This is my symbol definition:

auto box1=new QCheckBox;
box1->setText("choice1");
auto box2=new QCheckBox;
box2->setText("choice2")
QPushButton* button=new QPushButton;
connect(button,&QPushButton::clicked,this,&Widget::onClicked);
auto group=new QButtonGroup;
group->setExclusive(false);
group->addButton(box1);
group->addButton(box2);

void Widget::onClicked()
{
    std::for_each(group->buttons().begin(),group->buttons().end(),[](QAbstractButton* button){
            button->setChecked(!button->isChecked());
    });
    /*
    QList<QAbstractButton *> buttons=group->buttons();
    std::for_each(buttons.begin(),buttons.end(),[](QAbstract* button){
    button->setChecked(!button->isChecked());
    });
    */
}

The buttons() call will return a QList<QAbstractButton *>.

I tried to use onClicked slot to iterate though the list and do something.

If I add the uncommented section of code, the program crashes. I debugged it and found out that it looped three times, but only two elements were actually added, and the third loop will crash the program.

And if I remove the commented out piece of code, it works.

I want to know why the first way of writing, that is, the code that is not commented out, will cause an error.

Answer 1

As I explained in comments (not checked myself but confirmed by @musicamente), std::for_each(group->buttons().begin(), group->buttons().end(), [...]) creates 2 separate lists (by copy construction), the end() iterator of which are different; importantly, you cannot reach the end() iterator of the latter from the begin() iterator of the former, which causes std::for_each to keep looping past the end.

This issue would not have occurred with a range-based loop (which is easier to read to top). While I am at it, I will use the toggle method to switch the checkstate of checkboxes:

void Widget::onClicked()
{
    for (auto checkbox: group->buttons())
        button->toggle();
}

On a separate note, unless you have a specific reason to do it the way you present in your question,such as @musicamente's comment below (but even then, I had rather create a connection every time I create a new checkbox at run-time + disconnection is automatic when objects are deleted), the correct way of connecting your button to your many checkboxes would be:

[...]
QPushButton* button=new QPushButton;
auto group=new QButtonGroup;
group->setExclusive(false);
group->addButton(box1);
group->addButton(box2);
for (auto checkbox : group->buttons())
    QObject::connect(button, &QAbstractButton::clicked, checkbox, &QAbstractButton::toggle);

This saves declaring void Widget::onClicked().

Depending on the rest of the Widget class, it could even work without group, in that fashion:

[...]
QPushButton* button=new QPushButton;
for (auto checkbox : findChildren<QCheckBox>(QString()))
    QObject::connect(button, &QAbstractButton::clicked, checkbox, &QAbstractButton::toggle);