Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[desktop] More fixes for Windows #7621

Merged
merged 18 commits into from
Apr 4, 2024
Merged

Conversation

Osyotr
Copy link
Contributor

@Osyotr Osyotr commented Mar 17, 2024

Moving changes from this branch: https://github.com/Osyotr/organicmaps/tree/wip-windows

See commit messages for explanations.

@Osyotr Osyotr force-pushed the windows-fixes branch 2 times, most recently from 05f4ca3 to 80d06d7 Compare March 17, 2024 23:52
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for a huge work on the porting!

indexer/ftraits.hpp Outdated Show resolved Hide resolved
@@ -5,6 +5,15 @@ set(CMAKE_WARN_DEPRECATED OFF CACHE BOOL "" FORCE)
if (WITH_SYSTEM_PROVIDED_3PARTY)
set(GFLAGS_USE_TARGET_NAMESPACE ON)
find_package(gflags REQUIRED GLOBAL)

set(_backup_CMAKE_FIND_PACKAGE_TARGETS_GLOBAL ${CMAKE_FIND_PACKAGE_TARGETS_GLOBAL})
set(CMAKE_FIND_PACKAGE_TARGETS_GLOBAL ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supported only from cmake 3.24, for safety, it's better to use the syntax above with REQUIRED GLOBAL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find_package(... GLOBAL) is also CMake 3.24 feature. Maybe it's time to bump CMake version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I see... No, we can't increase it yet due to lower Android cmake version.

What is the reason for promoting packages to a global scope? What are other possible solutions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't increase it yet due to lower Android cmake version

Why can't it be updated? I mean, isn't cmake used to build android version is the same as the one used to build desktop version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for promoting packages to a global scope? What are other possible solutions here?

find_package results are not visible outside of current directory. Possible solution: move package searching to the root directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is shipped with Android SDK.

It's shipped for convenience; you can use any cmake you want https://developer.android.com/studio/projects/install-ndk#vanilla_cmake

It would be easier to merge C++ fixes separately from cmake fixes that may be more complex.

Ok, I'll move C++ changes into a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use any cmake you want

That's not easy. Google heavily patched their cmake in the past to work properly with android. Forcing all android devs to setup a custom sideloaded cmake and crossing fingers for it to work is not an option.

We may consider cmake upgrade later when Android SDK officially updates it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm https://developer.android.com/ndk/guides/cmake

Warning: CMake has its own built-in NDK support. Before CMake 3.21, this workflow is not supported by Android and is often broken with new NDK releases. Starting from CMake 3.21, the implementations are merged. CMake's built-in support has similar behavior to the NDK toolchain file, though variable names differ. Starting from Android NDK r23, the toolchain file will delegate to CMake's built-in support when using CMake 3.21 or later. See Issue 463 for more information. Note that the Android Gradle Plugin still uses the NDK's toolchain file automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it help to automatically provide the correct CMake version/binary for Android Studio users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not. It just means, "Google heavily patched their cmake in the past to work properly with android" is no longer a concern.
PS: I don't intend to force CMake upgrade, your arguments are very convincing.

set(_backup_CMAKE_FIND_PACKAGE_TARGETS_GLOBAL ${CMAKE_FIND_PACKAGE_TARGETS_GLOBAL})
set(CMAKE_FIND_PACKAGE_TARGETS_GLOBAL ON)

find_package(expat CONFIG REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for building these packages as external ones? Can windows build be done without defining WITH_SYSTEM_PROVIDED_3PARTY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for building these packages as external ones?

With WITH_SYSTEM_PROVIDED_3PARTY and without find_package(expat) cmake will treat target_link_libraries(foo PRIVATE expat) as -lexpat which is incorrect (library name on windows is libexpat/libexpatd).

Can windows build be done without defining WITH_SYSTEM_PROVIDED_3PARTY?

Probably, but I didn't test it as it requires external Qt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug that can be fixed separately, no?

@@ -18,6 +27,7 @@ else()
set(EXPAT_ENABLE_INSTALL OFF)
set(EXPAT_SHARED_LIBS OFF)
add_subdirectory(expat/expat)
add_library(expat::expat ALIAS expat)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be either add_subdirectory or add_library used, instead of using both, here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If expat is consumed using cmake config, then the target name is expat::expat.
If vendored expat is used, then the target name does not contain namespace expat::.
This change makes sure that consumer code works regardless of which mechanism is used.

3party/opening_hours/opening_hours.cpp Outdated Show resolved Hide resolved
platform/platform_win.cpp Outdated Show resolved Hide resolved
@@ -95,6 +95,12 @@ struct Polygon
Polygon(m2::RectD const & rect, std::vector<MarkedPoint> && points)
: m_rect(rect), m_points(std::move(points)) {}

Polygon(Polygon const &) noexcept = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which code requires these constructors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly reduced error message (brace yourself):

xmemory(748): error C2280: 'poly_borders::MarkedPoint::MarkedPoint(const poly_borders::MarkedPoint &)': attempting to reference a deleted function
poly_borders/help_structures.hpp(90): note: compiler has generated 'poly_borders::MarkedPoint::MarkedPoint' here
poly_borders/help_structures.hpp(90): note: 'poly_borders::MarkedPoint::MarkedPoint(const poly_borders::MarkedPoint &)': function was implicitly deleted because a data member invokes a deleted or inaccessible function 'std::unique_ptr<std::mutex>::unique_ptr(const std::unique_ptr<std::mutex> &)'
memory(3451): note: 'std::unique_ptr<std::mutex>::unique_ptr(const std::unique_ptr<std::mutex> &)': function was explicitly deleted
xmemory(748): note: the template instantiation context (the oldest one first) is
poly_borders/help_structures.hpp(112): note: see reference to class template instantiation 'std::vector<poly_borders::MarkedPoint>' being compiled
vector(676): note: while compiling class template member function 'std::vector<poly_borders::MarkedPoint>::vector(const std::vector<poly_borders::MarkedPoint> &)'
poly_borders/help_structures.hpp(115): note: see the first reference to 'std::vector<poly_borders::MarkedPoint>::vector' in 'poly_borders::Polygon::Polygon'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't MarkedPoint copy constructor and operator= be explicitly deleted, as they have an unique_ptr member?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly deleting MarkedPoint copy constructor and assignment operator (and adding defaulted move variants) yields pretty much the same error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which code requires copy constructor and operator= for Polygon class? I see it's moved in the code.

Copy link
Contributor Author

@Osyotr Osyotr Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an MSVC bug.
https://godbolt.org/z/GcK7WPr87

@@ -185,6 +185,7 @@ set(SRC
omim_add_library(${PROJECT_NAME} ${SRC})

target_link_libraries(${PROJECT_NAME}
base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, base library should come as a dependency from other referenced libraries below. Why it doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably related to the fact that some target_link_libraries calls in this project don't have PUBLIC/PRIVATE keywords.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it should be fixed!

@@ -138,8 +138,11 @@ RoutesBuilder::Result RoutesBuilder::ProcessTask(Params const & params)

std::future<RoutesBuilder::Result> RoutesBuilder::ProcessTaskAsync(Params const & params)
{
Processor processor(m_numMwmIds, m_dataSourcesStorage, m_cpg, m_cig);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why MSVC compiler didn't like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto MSVC STL bug

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a comment here, because code is a bit strange now

search/feature_offset_match.hpp Show resolved Hide resolved
@biodranik
Copy link
Member

Did you check why tests for Mac are failing?

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 19, 2024

Did you check why tests for Mac are failing?

I don't have a clue about these failures. Nothing in this PR touches download functionality.

@@ -5,6 +5,15 @@ set(CMAKE_WARN_DEPRECATED OFF CACHE BOOL "" FORCE)
if (WITH_SYSTEM_PROVIDED_3PARTY)
set(GFLAGS_USE_TARGET_NAMESPACE ON)
find_package(gflags REQUIRED GLOBAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We can handle it differently. On Android an older cmake is used. And usually, nobody defines WITH_SYSTEM_PROVIDED_3PARTY for android.
    Then we can check the cmake version here and fail if it's not supported.

  2. If WITH_SYSTEM_PROVIDED_3PARTY is the only way to build for windows, then we should enforce that.

  3. It would be great to move cmake-related changes to a different PR (or even separate PRs).

@Osyotr Osyotr marked this pull request as draft March 20, 2024 12:35
@Osyotr Osyotr force-pushed the windows-fixes branch 2 times, most recently from 1235e80 to 5f1d8ff Compare March 20, 2024 21:56
@Osyotr Osyotr marked this pull request as ready for review March 20, 2024 22:45
@biodranik
Copy link
Member

@Osyotr did you try to enable C++20 for msvc? If it works, we can avoid string view changes, right?

And if it doesn't work, what should be changed?

@biodranik
Copy link
Member

Should polygon constructors commits be squashed?

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 22, 2024

@Osyotr did you try to enable C++20 for msvc?

Yes. FYI, here's the build log.
organicmaps-win-build-log.txt

If it works, we can avoid string view changes, right?

No:

  • res.ptr == sv.end()); is invalid (comparison of pointer and iterator)
  • sv = std::string_view(beg, std::distance(beg, end)); is invalid (no constructor taking iterator and size)

Though we can probably avoid further string_utils.hpp changes, i.e. remove non-portable hack in TokenizeIteratorBase.

@Osyotr Osyotr force-pushed the windows-fixes branch 2 times, most recently from d7cb480 to 5ab3620 Compare March 23, 2024 10:12
@@ -55,7 +55,7 @@ bool ParseEmoji(CategoriesHolder::Category::Name & name)

void FillPrefixLengthToSuggest(CategoriesHolder::Category::Name & name)
{
if (std::isdigit(name.m_name.front()) && name.m_name.front() != '0')
if (std::isdigit(static_cast<unsigned char>(name.m_name.front())) && name.m_name.front() != '0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Osyotr
Those casts to unsigned char break the tests on Mac. They hang on parsing the arabic UTF-8 string by not seeing a valid UTF-8 character.

These functions should take a signed int type instead of unsigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for investigating.
Cppreference explicitly mentions that passing something that is not representable as unsigned char is wrong.
https://en.cppreference.com/w/cpp/string/byte/isdigit
I'll look further into it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature has accepted int since ancient C times: int isdigit( int ch ); so using int should be enough.

Yes, it is interesting to understand why it hangs.

This code returned 0:

    template <typename octet_iterator>
    inline typename std::iterator_traits<octet_iterator>::difference_type
    sequence_length(octet_iterator lead_it)
    {
        uint8_t lead = utf8::internal::mask8(*lead_it);
        if (lead < 0x80)
            return 1;
        else if ((lead >> 5) == 0x6)
            return 2;
        else if ((lead >> 4) == 0xe)
            return 3;
        else if ((lead >> 3) == 0x1e)
            return 4;
        else
            return 0;
    }

So this loop in Move was indefinite:

    while (m_end != m_finish && !m_delimFn(*m_end))
      ++m_end;

It is called when search categories are loaded in void CategoriesHolder::LoadFromStream(std::istream & s)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're using unsafe utf8 iteration without checks/crashes in cases of invalid text, somehow iterator pointed to a char value in the string that could not be incremented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Osyotr does it make sense to change the code to int, merge PR, and proceed with the investigation separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to move this commit to another PR. I don't think the change is wrong.
I'll do it later today (or your can do it now since you've got write access to this PR).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hanging code on Mac either proves that it's wrong or hints about a bug in the recent Mac OS clang.

Any help with PRs is greatly appreciated 🙌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Osyotr the failing line contains arabic chars, I see this in debugger:

"ar:طعام|أمكان لتناول الطعا\xd9"

It's line 80 from categories.txt

ar:طعام|أمكان لتناول الطعام

m_start says zero, m_end is strange
выява

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 30, 2024

CI is green.
@biodranik since you seem to be able to reproduce the issue locally, would you kindly provide the exact input string that triggers the infinite loop in TokenizeIterator?

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@vng PTAL

@@ -41,7 +41,7 @@ namespace succinct {
bit_vector_builder(uint64_t size = 0, bool init = 0)
: m_size(size)
{
m_bits.resize(detail::words_for(size), uint64_t(-init));
m_bits.resize(detail::words_for(size), uint64_t(-static_cast<int64_t>(init)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very strange notation.

  • Make bool init = false
  • Make:
init ? uint64_t(-1) : 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated assembly is pretty much the same but your code is much easier to read.
https://godbolt.org/z/34qWGbbK4

base/timer.cpp Outdated
auto const duration = system_clock::now().time_since_epoch();
auto const sec = duration_cast<seconds>(duration);
auto const usec = duration_cast<microseconds>(duration - sec);
return sec.count() + usec.count() / 1000000.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple std::chrono::duration<double>(duration).count() should do the trick

@@ -95,6 +95,9 @@ struct Polygon
Polygon(m2::RectD const & rect, std::vector<MarkedPoint> && points)
: m_rect(rect), m_points(std::move(points)) {}

Polygon(Polygon &&) = default;
Polygon & operator=(Polygon &&) = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why we should declare it explicitly?
  • Put noexcept

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we should declare it explicitly?

#7621 (comment)

Put noexcept

Looks like only operator= can be marked noexcept. Neither libstdc++ nor MSVC STL have noexcept move constructor for std::set:
https://github.com/gcc-mirror/gcc/blob/b313baba57f7e09f66b603e1e30dd4b48800693f/libstdc%2B%2B-v3/include/bits/stl_set.h#L233
https://github.com/microsoft/STL/blob/be81252ed1f5e5fc6d77faca0b5dbbbdae8143a2/stl/inc/set#L142

@@ -138,8 +138,11 @@ RoutesBuilder::Result RoutesBuilder::ProcessTask(Params const & params)

std::future<RoutesBuilder::Result> RoutesBuilder::ProcessTaskAsync(Params const & params)
{
Processor processor(m_numMwmIds, m_dataSourcesStorage, m_cpg, m_cig);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a comment here, because code is a bit strange now

SearchTrieRequest & operator=(SearchTrieRequest const &) = delete;

SearchTrieRequest(SearchTrieRequest &&) = default;
SearchTrieRequest & operator=(SearchTrieRequest &&) = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Is it needed to explicitly declare ctors and moves?
  • Put noexcept

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move constructors are needed to make MSVC happy:

  FAILED: search/CMakeFiles/search.dir/geocoder.cpp.obj 
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\xmemory(748): error C2280: 'strings::LevenshteinDFA::LevenshteinDFA(const strings::LevenshteinDFA &)': attempting to reference a deleted function
  base/levenshtein_dfa.hpp(97): note: see declaration of 'strings::LevenshteinDFA::LevenshteinDFA'
  base/levenshtein_dfa.hpp(97): note: 'strings::LevenshteinDFA::LevenshteinDFA(const strings::LevenshteinDFA &)': function was explicitly deleted
  C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\xmemory(748): note: the template instantiation context (the oldest one first) is
  search/ranking_utils.hpp(190): note: see reference to class template instantiation 'std::vector<strings::LevenshteinDFA,std::allocator<strings::LevenshteinDFA>>' being compiled
  C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\vector(676): note: while compiling class template member function 'std::vector<strings::LevenshteinDFA,std::allocator<strings::LevenshteinDFA>>::vector(const std::vector<strings::LevenshteinDFA,std::allocator<strings::LevenshteinDFA>> &)'
  search/feature_offset_match.hpp(224): note: see the first reference to 'std::vector<strings::LevenshteinDFA,std::allocator<strings::LevenshteinDFA>>::vector' in 'search::SearchTrieRequest<strings::LevenshteinDFA>::SearchTrieRequest'
  C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\vector(680): note: see reference to function template instantiation 'void std::vector<strings::LevenshteinDFA,std::allocator<strings::LevenshteinDFA>>::_Construct_n<strings::LevenshteinDFA*const &,strings::LevenshteinDFA*const &>(const unsigned __int64,strings::LevenshteinDFA *const &,strings::LevenshteinDFA *const &)' being compiled
  C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\vector(2078): note: see reference to function template instantiation 'strings::LevenshteinDFA *std::_Uninitialized_copy<strings::LevenshteinDFA*,strings::LevenshteinDFA*,std::allocator<strings::LevenshteinDFA>>(_InIt,_Se,strings::LevenshteinDFA *,_Alloc &)' being compiled
          with
          [
              _InIt=strings::LevenshteinDFA *,
              _Se=strings::LevenshteinDFA *,
              _Alloc=std::allocator<strings::LevenshteinDFA>
          ]
  C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\xmemory(1882): note: see reference to function template instantiation 'void std::_Uninitialized_backout_al<std::allocator<strings::LevenshteinDFA>>::_Emplace_back<strings::LevenshteinDFA&>(strings::LevenshteinDFA &)' being compiled
  C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.40.33617\include\xmemory(1828): note: see reference to function template instantiation 'void std::_Default_allocator_traits<_Alloc>::construct<_Ty,strings::LevenshteinDFA&>(_Alloc &,_Objty *const ,strings::LevenshteinDFA &)' being compiled
          with
          [
              _Alloc=std::allocator<strings::LevenshteinDFA>,
              _Ty=strings::LevenshteinDFA,
              _Objty=strings::LevenshteinDFA
          ]

Put noexcept

Nope. std::is_nothrow_move_assignable_v<strings::UniStringDFA> is false and std::unordered_set does not have noexcept move constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes, because we should make buffer_vector noexcept first in other PR :)

@vng
Copy link
Member

vng commented Mar 30, 2024

Is it possible to make Windows Actions right in this PR?

@biodranik
Copy link
Member

Is it possible to make Windows Actions right in this PR?

This PR doesn't fix all Windows issues yet. @vng PTAL

@vng
Copy link
Member

vng commented Mar 31, 2024

Honestly, I don't want to merge it now (some fixes are MS crutches) until see what these efforts were for ..

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 31, 2024

Honestly, I don't want to merge it now (some fixes are MS crutches) until see what these efforts were for ..

Not sure what you are expecting to see but I can move those crutches into a separate PR.

@biodranik
Copy link
Member

Honestly, I don't want to merge it now (some fixes are MS crutches) until see what these efforts were for ..

We need to unlock windows builds, step by step. There are many other changes needed.

It should be fine to explicitly mark some MSVC issues in comments, so we can always come back and revise it later.

@@ -41,7 +41,7 @@ namespace succinct {
bit_vector_builder(uint64_t size = 0, bool init = 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bit_vector_builder(uint64_t size = 0, bool init = 0)
bit_vector_builder(uint64_t size = 0, bool init = false)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also renamed it to initBit because in my mind bool init means "whether to init this or not" instead of "what value to use to init"

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok to merge it, two minor nits left

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Osyotr added 17 commits April 4, 2024 00:41
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
…s builds

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Note: constructing std::string_view from iterators is OK in C++20, but we are not there yet.
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
error C2672: 'PrintPaddedNumber': no matching overloaded function found

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Fixes usage of move-only types with this class

Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
Signed-off-by: Osyotr <Osyotr@users.noreply.github.com>
@Jean-BaptisteC Jean-BaptisteC changed the title More fixes for Windows [desktop] More fixes for Windows Apr 4, 2024
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray, thanks a lot!

@biodranik biodranik merged commit 2760ded into organicmaps:master Apr 4, 2024
15 checks passed
@biodranik
Copy link
Member

@Osyotr what's next? )

@Osyotr Osyotr deleted the windows-fixes branch April 6, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants