Freeciv

Is this really needed for S2_6?

Maybe ok to ignore the S2_6 patch. We may run at least one 2.6 cross-platform game before 3.0 releases are stable.

This introduces callback functions inside the body of the other function. Our CodingStyle has nothing to say about that, so I'll give people a bit time to comment.

Note: Avoid using this function; instead, use open(). Unlike exec(), open() is asynchronous, and does not spin an additional event loop.

https://doc.qt.io/qt-5/qdialog.html#exec

Up for discussion which C++11 features we want to allow, but I think lambdas make the code more readable here.

C++11 lambdas are the way to go in this context. Their use is also encouraged by Qt.

However, one should be very careful when capturing pointers, since the pointed-to memory may be freed between capture and execution. It looks like the following code may lead to a crash:

QObject::connect(ask, &hud_message_box::finished, [=](int ret) {
  if (ret == QMessageBox::Ok) {
    if (!pcity->did_sell && city_has_building(pcity, building)) {
      city_sell_improvement(pcity, improvement_number(building));
    }
  }
});

Consider the following sequence:

• Start selling building in city; confirmation popup appears
• City is destroyed, freeing *pcity
• Confirmation popup is accepted and tries to access *pcity

I say allow lambdas but discourage pointer capture.

As per Feature #851519 findings, only master version should use lambdas. S3_0 version should not, assuming this patch is wanted there.

    QObject::connect(ask, &hud_message_box::finished, [=](int ret) {
  QObject::connect(ask, &hud_message_box::finished, [=](int ret) {

    int which_menu = qvar.toInt();
    int c_id = qvar.toInt();

    which_menu = qvar.toInt();
    qvar = act->data();
    c_id = qvar.toInt();

            QObject::connect(ask, &hud_message_box::finished, [=](int ret) {
              if (ret == QMessageBox::Ok) {
                if (!pcity->did_sell && city_has_building(pcity, building)) {
                  city_sell_improvement(pcity, improvement_number(building));
                }
              }
        });

    Q_ASSERT(col >= 0);

  diplomat_queue_handle_secondary(diplomat_id);

void eco_report::sell_buildings()

{

FTR two possible bugs found while reviewing this patch:

[14:24] <louis94> I notice that city_dialog keeps a pointer to the city struct
[14:24] <louis94> This is probably fine most of the time but in some cases not
[14:26] <louis94> Think city is removed, city dialog is popdown is triggered and struct is deleted, but there was still something queued in the event loop
[14:26] <louis94> In this case I think Qt will still deliver the event before popdown
[14:26] <louis94> But by then the struct will be gone...
[14:27] <louis94> I didn't do anything to confirm this and don't even have a clear idea how

[14:42] <louis94> Also, I see char* being passed to Qt, no encoding problem there? In the other direction we use toLocal8Bits()

Looks good to me, ship it!