Remove std::vector<bool>

[fwesselm]

Replace std::vector<bool> by std::vector<uint8_t>.

I noticed that the format of some files was changed. I can undo this if needed.

[codecov]

Codecov Report

❌ Patch coverage is 75.94937% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.89%. Comparing base (1504344) to head (1951fc2).
⚠️ Report is 1 commits behind head on latest.

Files with missing lines	Patch %	Lines
highs/simplex/HSimplexNlaProductForm.cpp	0.00%	4 Missing ⚠️
highs/lp_data/HighsInterface.cpp	75.00%	2 Missing ⚠️
highs/mip/HighsSearch.cpp	66.66%	2 Missing ⚠️
highs/presolve/HPresolve.cpp	0.00%	2 Missing ⚠️
highs/util/HFactorRefactor.cpp	0.00%	2 Missing ⚠️
highs/ipm/hipo/factorhighs/Analyse.cpp	0.00%	1 Missing ⚠️
highs/ipm/hipo/ipm/FactorHighsSolver.cpp	0.00%	1 Missing ⚠️
highs/ipm/hipo/ipm/Model.cpp	0.00%	1 Missing ⚠️
highs/ipm/hipo/ipm/UpLookingSolver.cpp	0.00%	1 Missing ⚠️
highs/ipm/ipx/lu_factorization.cc	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@           Coverage Diff           @@
##           latest    #3197   +/-   ##
=======================================
  Coverage   72.89%   72.89%           
=======================================
  Files         441      441           
  Lines      106366   106364    -2     
  Branches    17120    17120           
=======================================
+ Hits        77536    77538    +2     
+ Misses      28553    28549    -4     
  Partials      277      277           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[galabovaa]

Thank you @fwesselm! Yes please, can you undo the formatting? We are not checking ipx code in the clang format, but it would be good for clarity

Also, can you please merge this commit: ec0882b

or branch bazel-fail-testlogs-tsan

So the tsan logs would be uploaded if the sanitizer fails again? These failures only pop up very rarely and it would be good to investigate them

[fwesselm]

Thank you @fwesselm! Yes please, can you undo the formatting? We are not checking ipx code in the clang format, but it would be good for clarity

Also, can you please merge this commit: ec0882b

or branch bazel-fail-testlogs-tsan

So the tsan logs would be uploaded if the sanitizer fails again? These failures only pop up very rarely and it would be good to investigate them

Thank you @galabovaa! I have reverted the format changes and made the merge.

[jajhall]

All std::vector<bool> have been replaced, but I've spotted

util/HFactorRefactor.cpp:43: vector has_pivot;
util/HFactorRefactor.cpp:212: vector not_in_bump = has_pivot;

which aren't std::vector, so these should be replace (and written as std::, even if unnecessary!

[fwesselm]

All std::vector<bool> have been replaced, but I've spotted

util/HFactorRefactor.cpp:43: vector has_pivot;
util/HFactorRefactor.cpp:212: vector not_in_bump = has_pivot;

which aren't std::vector, so these should be replace (and written as std::, even if unnecessary!

@jajhall, I made these changes.

In general, there are many occurrences of unqualified vector<...> across many files. Would you also like to have these explicitly written as std::?

[jajhall]

In general, there are many occurrences of unqualified vector<...> across many files. Would you also like to have these explicitly written as std::?

Not for now, as it may create merge conflicts

[BenChampion]

The following is almost certain to be more trouble that it's worth, but it'd be nice to do something like typedef uint8_t highs_bool_t; (or using bool_t = std::uint8_t; in the highs namespace if this only affects C++ code) somewhere and then use highs_bool_t for all these occurrences.

The problem is that everything would have to include whatever header file would incorporate this definition. If there isn't already such a file, I think it may not be worth doing.

[jajhall]

The following is almost certain to be more trouble that it's worth, but it'd be nice to do something like typedef uint8_t highs_bool_t; (or using bool_t = std::uint8_t; in the highs namespace if this only affects C++ code) somewhere and then use highs_bool_t for all these occurrences.

The problem is that everything would have to include whatever header file would incorporate this definition. If there isn't already such a file, I think it may not be worth doing.

I see this as having the advantage of clarity, in that the old std::vector<bool> would still have "bool-ness" in the type. If std::uint8_ is used as the type, then there may be some doubt over whether entries can be set to something other than static_cast<std::uint8_>(false) or static_cast<std::uint8_>(true).

We have the highs/util/HighsInt.hheader file that is used to define HighsInt that could be generalised (and renamed) to include typedef uint8_t highs_bool_t;

[fwesselm]

The following is almost certain to be more trouble that it's worth, but it'd be nice to do something like typedef uint8_t highs_bool_t; (or using bool_t = std::uint8_t; in the highs namespace if this only affects C++ code) somewhere and then use highs_bool_t for all these occurrences.
The problem is that everything would have to include whatever header file would incorporate this definition. If there isn't already such a file, I think it may not be worth doing.

I see this as having the advantage of clarity, in that the old std::vector<bool> would still have "bool-ness" in the type. If std::uint8_ is used as the type, then there may be some doubt over whether entries can be set to something other than static_cast<std::uint8_>(false) or static_cast<std::uint8_>(true).

We have the highs/util/HighsInt.hheader file that is used to define HighsInt that could be generalised (and renamed) to include typedef uint8_t highs_bool_t;

@BenChampion, @jajhall, thanks for your comments! I have added HighsBool to the header.

[jajhall]

@BenChampion, @jajhall, thanks for your comments! I have added HighsBool to the header.

I see that you've changed all std::vector<uint8_t> to std::vector<HighsBool>, except one! If this means that you've considered whether the std::vector<uint8_t> in all cases was being used as a bool, ratherthan a short integer, then bravo!

For clarity, shouldn't the name of highs/util/HighsInt.h change to highs/util/HighsType.h, now?

[fwesselm]

@BenChampion, @jajhall, thanks for your comments! I have added HighsBool to the header.

I see that you've changed all std::vector<uint8_t> to std::vector<HighsBool>, except one! If this means that you've considered whether the std::vector<uint8_t> in all cases was being used as a bool, ratherthan a short integer, then bravo!

For clarity, shouldn't the name of highs/util/HighsInt.h change to highs/util/HighsType.h, now?

@jajhall, I hope I did not miss a vector. I have renamed the header.

Note that uno tests are failing because they include the old header.

[jajhall]

@jajhall, I hope I did not miss a vector. I have renamed the header.

Note that uno tests are failing because they include the old header.

Ah, I'd not considered that HighsInt.h might be used externally. Other users may be doing this (as well as @cvanaret).

The name change should be reverted, and HighsInt.h included in a new header HighsType.h

[fwesselm]

@jajhall, I hope I did not miss a vector. I have renamed the header.
Note that uno tests are failing because they include the old header.

Ah, I'd not considered that HighsInt.h might be used externally. Other users may be doing this (as well as @cvanaret).

The name change should be reverted, and HighsInt.h included in a new header HighsType.h

I have reverted the move and added a new header HighsType.h.

[jajhall]

@jajhall, I hope I did not miss a vector.

The only occurrences of the string <bool> in HiGHS are in the context of std::atomic and std::valarray (and there no occurrences of <std::bool>), so you've not missed any vectors!

I've been through the use of <HighsBool>, and I don't think it's used in any case where the vector has values other than true or false assigned to it. There are places where vectors of the same name are defined as HighsBool and uint8_t, but the latter is correct because +1 and -1 are assigned. Of course it's fail-safe since HighsBool is uint8_t: it's just a matter of code clarity.

There are places in the MIP solver where, I guess, std::vector<bool> had previously been changed to std::vector<uint8_t> and are now std::vector<HighsBool>, but are treated as if they contained integers, with statements like != 0 rather than true. All fail-safe, though, @Opt-Mucca

I've added comment to util/HighsType.h about the motivation for HighsBool, and (@filikat) changed std::vector<char> failure(k, 0); to std::vector<HighsBool> failure(k, false); in FactorHighsSolver.cpp

So, unless there are any further queries, this is good to go!

[Opt-Mucca]

@jajhall that's correct. There's a fair few std::vector<uint8_t> vectors used locally in HighsMipSolver.cpp that were done for parallel MIP. I think I jumped between val != 0 and if(val) sometimes.

The PR is good to go from me!

[fwesselm]

@galabovaa, should I revert the change to workflows/action-sanitizers-bazel.yml?

[BenChampion]

Very nice change! Thanks for going the extra 1.6 km with the changes from 0/1 to true/false and 1 - ... to !...

Perhaps a bit paranoid, but it seems worth performance-testing this change, just to be sure. (My expectation is no change. This is just to catch any weird hard-to-foresee stuff, like how the compiler optimizes HighsBool -> bool and bool -> HighsBool, any memory alignment differences, that sort of thing.)

[jajhall]

Very nice change! Thanks for going the extra 1.6 km with the changes from 0/1 to true/false and 1 - ... to !...

Perhaps a bit paranoid, but it seems worth performance-testing this change, just to be sure. (My expectation is no change. This is just to catch any weird hard-to-foresee stuff, like how the compiler optimizes HighsBool -> bool and bool -> HighsBool, any memory alignment differences, that sort of thing.)

My instinct is that the overhead of using these boolean vectors is very small, so the change in the way that they are represented is of no consequence. If we're ever in any doubt, we can rename HighsBool to bool and do a test

With my own paranoia, I also did a search for "class" and "bool" in the same line of HiGHS in case there was an "enum class Name : bool" anywhere that might be used to type an std::vector. There's none.

I did spot that there are many enum class definitions that don't refer to uint8_t. Assuming that they don't have more than 256 members, is there anything lost or gained by inserting : uint8_t? Perhaps the optimizing compiler does this anyway?