Skip to content

Conversation

@flomnes
Copy link
Contributor

@flomnes flomnes commented Sep 20, 2024

Using such functions as std::stoi, std::stod, etc. caused us a lot of trouble because these functions are locale dependent, as illustrated below

#include <iostream>
#include <locale>
#include <string>

void convert(std::string locale)
{
    std::string value = "9.81";
    if (std::setlocale(LC_ALL, locale.c_str()))
    {
        double converted = std::stod(value);
        std::cout << "locale " << locale << " converted to " << converted << std::endl;
    }
    else
    {
        std::cout << "locale set failed for " << locale << std::endl;
    }
}
int main()
{
    convert("C");
    convert("fr_FR.utf8");
    // OUTPUT
    // locale C converted to 9.81
    // locale fr_FR.utf8 converted to 9
}

At first, we thought that using ScopedLocale would allow us to temporarily use the "C" locale, but this approach has multiple problems

  1. It's not thread-safe. For example calling std::stod when the locale is being changed through std::setlocale is undefined behavior source.
  2. C is hard, const char* pointers easily become dangling or invalid without the caller knowing
struct ScopedLocale {
  ScopedLocale() {
    oldLocale = std::setlocale(LC_NUMERIC, nullptr);
    auto newLocale = std::setlocale(LC_NUMERIC, "C");
    CHECK_EQ(std::string(newLocale), "C");
  }
  ~ScopedLocale() { std::setlocale(LC_NUMERIC, oldLocale); }

 private:
  const char* oldLocale;
};

We ran into an issue where the locale wasn't properly restored after using MPSolver::XpressInterface. So we decided to use absl's locale independent functions to perform that task.

OMNES Florian and others added 2 commits September 20, 2024 16:22
Use absl's locale-independent conversion functions in XPRESS interface
@google-cla
Copy link

google-cla bot commented Sep 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@flomnes flomnes changed the title Use absl's locale-independent conversion functions in XPRESS interface Use absl's locale-independent conversion functions in XPRESS MPSolver interface Sep 20, 2024
@Mizux Mizux added Bug Solver: Linear Solver Related to all Linear Solver (GLOP, BOP, CBC etc...) labels Sep 20, 2024
@Mizux Mizux added this to the v10.0 milestone Sep 20, 2024
@lperron lperron merged commit 9f41024 into google:main Sep 21, 2024
@Mizux Mizux modified the milestones: v10.0, v9.12 Oct 22, 2024
@Mizux Mizux self-assigned this Mar 3, 2025
@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

Hi @lperron it seems that this fix was neither shipped into v9.11 nor v9.12 since it was merged into google:main.

Do you plan to ship it in the next version ?

@lperron
Copy link
Collaborator

lperron commented Jun 3, 2025 via email

@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

Can you check on the v99bugfix branch ?

Unfortunately the ScopedLocale class is still defined & used in branch v99bugfix (commit 6275619).

@lperron
Copy link
Collaborator

lperron commented Jun 3, 2025 via email

@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

And in main ? Sorry, I cannot check myself.

Yes, oddly enough ScopedLocale can also be found in google:main (commit a90d9d9)

@lperron
Copy link
Collaborator

lperron commented Jun 3, 2025 via email

@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

OK, we will re-apply the patch . Thanks for flagging.

You're welcome !

Mizux added a commit that referenced this pull request Jun 3, 2025
* Also add few fix from rte-france/or-tools fork (`main` branch)
@Mizux
Copy link
Collaborator

Mizux commented Jun 3, 2025

@flomnes can you check 1aad3b5
if OK I will cherry pick and apply it to main and v99bugfix branch of google/or-tools

note: I've used https://raw.githubusercontent.com/rte-france/or-tools/refs/heads/main/ortools/linear_solver/xpress_interface.cc as source of truth + clang-format --style=file -i ortools/linear_solver/xpress_interface.cc
aka see line 848 CHECK_STATUS() + void XpressInterface::Reset() rework

@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

@flomnes can you check 1aad3b5 if OK I will cherry pick and apply it to main and v99bugfix branch of google/or-tools

note: I've used https://raw.githubusercontent.com/rte-france/or-tools/refs/heads/main/ortools/linear_solver/xpress_interface.cc as source of truth + clang-format --style=file -i ortools/linear_solver/xpress_interface.cc aka see line 848 CHECK_STATUS() + void XpressInterface::Reset() rework

The proposed commit reverts the removal of XPRSloadlp's call in XpressInterface::Reset (see #4667), which we don't want. We only want to apply the removal of ScopedLocale (see #4382).

Mizux added a commit that referenced this pull request Jun 3, 2025
* Also add few fix from rte-france/or-tools fork (`main` branch)
@Mizux
Copy link
Collaborator

Mizux commented Jun 3, 2025

@flomnes can you check 1aad3b5 if OK I will cherry pick and apply it to main and v99bugfix branch of google/or-tools
note: I've used https://raw.githubusercontent.com/rte-france/or-tools/refs/heads/main/ortools/linear_solver/xpress_interface.cc as source of truth + clang-format --style=file -i ortools/linear_solver/xpress_interface.cc aka see line 848 CHECK_STATUS() + void XpressInterface::Reset() rework

The proposed commit reverts the removal of XPRSloadlp's call in XpressInterface::Reset (see #4667), which we don't want. We only want to apply the removal of ScopedLocale (see #4382).

Granted ! Update the patch 573927e
If OK will be merged asap in main and v99bugfix...

Mizux added a commit that referenced this pull request Jun 3, 2025
* Also add few fix from rte-france/or-tools fork (`main` branch)
Mizux added a commit that referenced this pull request Jun 3, 2025
* Also add few fix from rte-france/or-tools fork (`main` branch)
@Mizux
Copy link
Collaborator

Mizux commented Jun 3, 2025

@flommes should we backport #4667 to v99bugfix branch aka our next release v9.13 + soon stable branch ?

@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

Granted ! Update the patch 573927e
If OK will be merged asap in main and v99bugfix...

573927e looks good, I approve this cherry-pick.

@flomnes
Copy link
Contributor Author

flomnes commented Jun 3, 2025

@flommes should we backport #4667 to v99bugfix branch aka our next release v9.13 + soon stable branch ?

Yes, #4667 greatly improves performance for our use-cases so it would be nice to ship it in v9.13.

Thank you

@Mizux
Copy link
Collaborator

Mizux commented Jun 4, 2025

@flommes should we backport #4667 to v99bugfix branch aka our next release v9.13 + soon stable branch ?

Yes, #4667 greatly improves performance for our use-cases so it would be nice to ship it in v9.13.

Thank you

Cherry-pick done, hope to release this week !
ref: 6b83515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Solver: Linear Solver Related to all Linear Solver (GLOP, BOP, CBC etc...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants