Add ability to build with Clang #507

p-senichenkov · 2024-12-23T16:23:31Z

Make Desbordante be able to be built with Clang on Linux and LLVM/Apple CLang on macOS:

Make some changes to build.sh and CMakeLists.txt (bump versions of some libraries and workaround some Clang bugs)
Fix warnings, UB and address issues, that GCC hasn't found and Clang has
Update "build" section in README.md
Get rid of GCC intrinsics
Fix some implementation-defined behaviour
Add corresponding tests to CI
See commit descriptions for more information

Vdaleke

Thank you for your great work. I'm not completely done with the review yet and there will probably be more comments soon, but I'll post these so you can start fixing them.

I will leave a few comments that cannot be attached to individual files:

There are no binding-tests for Clang and macOS Apple Clang. And there may be problems similar to those that caused the Fix CMakeLists.txt, macOS guide and refactor .gitignore #492, I mean DYLD_PATH on macOS.
We keep a short commit history. Pls, squash commits to some parts. I propose squash into 4 like this: CI, src-fixing, CMake, README.
You made great descriptions for all commits, pls copy them in some way to PR description. If someone else looks at the PR, they don't look for commit descriptions, and it's also a good way to present the result.

.github/composite-actions/download-libraries/action.yml

.github/workflows/core-tests.yml

build.sh

.github/composite-actions/download-libraries/action.yml

README.md

.github/workflows/core-tests.yml

Vdaleke · 2025-01-04T19:07:53Z

Interesting checks, maybe somewhere into project settings these names are inlined to block merge without them?

p-senichenkov · 2025-01-04T19:12:42Z

Interesting checks, maybe somewhere into project settings these names are inlined to block merge without them?

Yes, it's branch protection rules. I left a comment about it in PR#499

Vdaleke · 2025-01-04T19:17:03Z

Interesting checks, maybe somewhere into project settings these names are inlined to block merge without them?

Yes, it's branch protection rules. I left a comment about it in PR#499

Oh, I didn't notice there are 2 pull request, is 499 needed at all if this one exists? I suggest closing it and continuing only in this one.

p-senichenkov · 2025-01-04T19:22:36Z

Oh, I didn't notice there are 2 pull request, is 499 needed at all if this one exists? I suggest closing it and continuing only in this one.

There's 3 pull requests. 499 addresses Clang on Linux, 500 -- LLVM Clang on macOS, and this PR addresses Apple Clang.
499 is a part of 500, and 500 is a part of 507. I recommend to review them in this order.
I think, it's better to have 3 different PRs and try to merge as many of them as possible before the release

Vdaleke · 2025-01-04T21:46:19Z

Oh, I didn't notice there are 2 pull request, is 499 needed at all if this one exists? I suggest closing it and continuing only in this one.

There's 3 pull requests. 499 addresses Clang on Linux, 500 -- LLVM Clang on macOS, and this PR addresses Apple Clang. 499 is a part of 500, and 500 is a part of 507. I recommend to review them in this order. I think, it's better to have 3 different PRs and try to merge as many of them as possible before the release

I don't like that they are tightly coupled in CI. It seems to me that when all the fixes are already in place, it's easier to do and review everything together, rather than merge one by one, changing the architecture in the process. And do we really need a release without a pip-package on MacOS?

Vdaleke

The second part of review.

As for constructors which is in a lot of comments. I heard about the performance of the aggregates from colleagues at work, and constantly. For example, that's why it is not recommended to use a std::pair, for some reason they added a constructor there and a regular handwritten structure will be faster. But I can't find normal proofs on the Internet.

src/core/util/bitset_extensions.cpp

src/core/util/bitset_extensions.h

src/core/util/maybe_unused.h

src/tests/test_algo_interfaces.cpp

src/tests/test_pfdtane.cpp

src/tests/test_tane_afd_measures.cpp

src/core/util/auto_join_thread.h

p-senichenkov · 2025-01-05T09:17:24Z

As for constructors which is in a lot of comments. I heard about the performance of the aggregates from colleagues at work, and constantly. For example, that's why it is not recommended to use a std::pair, for some reason they added a constructor there and a regular handwritten structure will be faster. But I can't find normal proofs on the Internet.

The only thing I can do is to define constructors conditionnally -- only when aggregate initialization isn't availible.

Vdaleke · 2025-01-05T13:29:39Z

As for constructors which is in a lot of comments. I heard about the performance of the aggregates from colleagues at work, and constantly. For example, that's why it is not recommended to use a std::pair, for some reason they added a constructor there and a regular handwritten structure will be faster. But I can't find normal proofs on the Internet.

The only thing I can do is to define constructors conditionnally -- only when aggregate initialization isn't availible.

I don't quite understand why you can't just use brace-initialization everywhere?

p-senichenkov · 2025-01-05T14:06:29Z

I don't quite understand why you can't just use brace-initialization everywhere?

Brace-initialization calls constructor. It won't help if we want to get rid of constructors.

Vdaleke · 2025-01-05T14:31:00Z

Brace-initialization calls constructor.

It's not true?

https://learn.microsoft.com/en-us/cpp/cpp/initializing-classes-and-structs-without-constructors-cpp?view=msvc-170#:~:text=When%20a%20class%20or%20struct%20has%20no%20constructor%2C%20you%20provide%20the%20list%20elements%20in%20the%20order%20that%20the%20members%20are%20declared%20in%20the%20class.

p-senichenkov · 2025-01-06T09:56:30Z

In terms of this page on cppreference, apparently, you suggest to use direct-list-initialization (2 variant) instead of copy-list-initialization (7 variant). Maybe this will work -- I'll check it later. But in most cases we have emplace_back, that cannot be replaced with direct-list-initialization.
So, there are still two possible ways:

conditionally define constructor
conditionally replace emplace_back(arg1, arg2) with push_back(T{arg1, arg2})
(by "conditionally", I mean using of feature-test macros). As for me, the second way is very bad.

Vdaleke · 2025-01-06T11:49:33Z

In terms of this page on cppreference,

Not working link

But in most cases we have emplace_back, that cannot be replaced with direct-list-initialization.

Use push_back instead

p-senichenkov · 2025-01-06T11:51:37Z

Not working link

https://en.cppreference.com/w/cpp/language/list_initialization

.github/composite-actions/install-dependencies/action.yml

github-actions

clang-tidy made some suggestions

src/core/util/bitset_extensions.h

Vdaleke · 2025-01-06T21:20:55Z

Due to ac2ad57.
Let me remind you that at the meeting we agreed to leave in the readme only gcc for Linux and only apple clang for macOS, everything else will move to the wiki.

github-actions

clang-tidy made some suggestions

src/core/algorithms/md/hymd/md_lhs.h

github-actions

clang-tidy made some suggestions

src/core/algorithms/md/hymd/md_lhs.h

github-actions

clang-tidy made some suggestions

src/core/algorithms/dd/split/split.cpp

Vdaleke

Thanks for the corrections, it looks much better. I left a couple of minor comments and questions. Of the possible major edits, only replacing action with a composition in CI-tests, if it is possible to catch an incorrect configuration.

.github/composite-actions/install-dependencies/action.yml

.github/workflows/bindings-tests.yml

.github/workflows/check-codestyle.yml

README.md

src/core/util/bitset_extensions.h

src/core/util/maybe_unused_on_clang.h

src/tests/test_algo_interfaces.cpp

Vdaleke · 2025-01-12T13:36:58Z

it was necessary to add jthread and the bitset extension only when apple-clang was added

No, it was necessary even on Linux (see #499)

Can't we use libstdc++?

We can, but there's no point in compiling with Clang, if we link it with GCC's standard library. In that case we could just close this PR without merging.

now I think that we should supply a pip package built with gcc even on macos, but that's not your concern anymore, maybe I'll add it myself

Vdaleke · 2025-01-30T10:24:41Z

Could you describe in more detail what error is happening and why, because I didn't quite understand what they were writing in the issue without context. The solution seems ok, but I would leave a comment on why we added CC export here, unlike other places.

p-senichenkov · 2025-01-30T10:31:06Z

Could you describe in more detail what error is happening and why, because I didn't quite understand what they were writing in the issue without context. The solution seems ok, but I would leave a comment on why we added CC export here, unlike other places.

The problem is, CMake somehow cannot locate clang-scan-deps tool when C compiler is set to GCC. I don't quite understand which update caused this error, but I'm sure it's not modern Ubuntu problem (see Ubuntu version in https://github.com/Desbordante/desbordante-core/actions/runs/13030263166/job/36347763368). As far as I understand from discussion of mentioned issue, this tool is needed to scan dependencies of C++ modules.

Vdaleke · 2025-01-30T10:41:42Z

Check this answer https://stackoverflow.com/a/77801270/16427597. CMake has variable to set path to clang-scan-deps and such a solution is at least more obvious than the fact that the C compiler helps him find it.

p-senichenkov · 2025-01-30T10:47:10Z

Check this answer https://stackoverflow.com/a/77801270/16427597. CMake has variable to set path to clang-scan-deps and such a solution is at least more obvious than the fact that the C compiler helps him find it.

This way I need to hard-code clang-scan-deps path. In that answer, they've first created symlink to the tool in /usr/local/bin, because it's location isn't in PATH by default. This solution will work only on one exact system.

Vdaleke · 2025-01-30T10:49:32Z

This way I need to hard-code clang-scan-deps path. In that answer, they've first created symlink to the tool in /usr/local/bin, because it's location isn't in PATH by default. This solution will work only on one exact system.

OK, then i think a comment will be enough

- Add `set -e` in build.sh to stop build process on error - Add SYSTEM property to all `add_subdirectory` commands that add external dependencies to avoid compiler warnings that aren't related to Desbordante - Disable alloc-dealloc-mismatch check as it's false positive on Ubuntu (see comment in CMakeLists.txt) - Limit UB sanitizer "function" check to only our code when building with Clang on macOS (see comment in CMakeLists.txt) - Update googletest version from 1.13.0 to 1.14.0, because 1.13.0 produces "narrowing conversion" warning due to a bug (issue #4206, google/googletest#4206) when building with Clang. This bug was fixed in 1.14.0 (commit 3044657, google/googletest@3044657) - Disable deprecated-declaration warning on Clang-18 (see comment in CMakeLists.txt)

Fix Clang warnings and UB and ADDRESS sanitizer errors: - Replace INSTANTIATE_TEST_CASE_P, which is deprecated, with INSTANTIATE_TEST_SUITE_P in NDVerifier tests - Mark unused fields as [[maybe_unused]]. Use MAYBE_UNUSED_PRIVATE_FIELD macro, because GCC produces warning when [[maybe_unused]] attribute is present, and Clang -- when it isn't. (see comment in util/maybe_unused_on_clang.h) - Convert double literals to model::DoubleType and model::IntType explicitly in tests/test_types.cpp - Check that column is numeric *before* casting it to INumericType in DataStats::GetMedianAD - Don't process empty vector in GetPartition in gfd_validation.cpp (immediately return 0) - Disable tests causing float-to-long cast when result is infinity in tests/test_types.cpp - Use std::chrono::high_resolution_clock instead of std::chrono::_V2::high_resolution_clock in fastod/util/timer.h, because symbol versioning is gcc-specific - Use &Insert instead of this->Insert in KDTree constructor - Use system_clock everywhere instead of mix of system_clock and high_resolution_clock in Cords::ExecuteInternal - Add fallback behaviour in hymd::utility::MdLessPairs if operator <=> for vectors isn't availible - Add explicit #include <string> in exceptions.h - Add template keyword in CanonicalOD::IsValid to avoid most vexing parse (see cross-platform guide in wiki) - Explicitly cast kSeed_ to result_type in des/rng.h, because result_type of mt19937 is always 32-bit and it's not bound to some built-in integral type - Use reverse iterators to traverse set in reverse order, instead of strange behaviour with direct iterators in egfd_validation.cpp/ReverseConstruction

Add information about other compilers in README. Extra build instructions (Linux & Clang, macOS & LLVM CLang, macOS & GCC) are now moved to wiki

- Use simple RAII wrapper of std::thread instead of std::jthread when the latter isn't implemented - Use custom implementations of std::bitset::_Find_first and _Find_next when gcc intrinsics aren't availible. Also, implement some utility functions using custom FindFirst and FindNext

Don't use features, which behaviour is implementation-defined and differ in libstdc++ and libc++. Instead use "stable" versions: - sort pairs by value if frequencies are equal in Cords' FrequencyHandler::InitFrequencyHandler - Use std::stable_sort instead of std::sort in DES::GetRandomPopulationInDomainds - Use std::set instead of std::unordered_set in differential_functions.cpp/GetRandIndices - Use std::stable_sort in PDFTane::CalculatePFDError instead of std::sort

Add tests for Clang on Linux, LLVM Clang on macOS and Apple Clang on macOS. Also, bump boost version used in CI, as 1.8.10 produces an error on macOS with LLVM Clang. Here it's said that boost 1.85.0 works fine: bambulab/BambuStudio#3957

Aggregate initialization isn't availible on Apple Clang. Use direct-list-initialization instead wherewer possible: - Split::IndexSearchSpace - MinPickerLattice::AddGeneralizations - OneByOnePicker::AddGeneralizations - md_lattice/Specializer - MdLattice::GetAll - hymd::LhsNode - ::BatchValidator - SimilarityData::Creator - BatchValidator::MultiCardPartitionElementProvider - MD::GetDescription - order::GetIndexedByteData - python_bindings::BindOd - model::DD - dd::DistancePositionListIndex - dc::DCCandidate Define constructor elsewhere: - test_algo_interfaces/KeysTestParams - test_dc_verifier/DCTestParams - test_ind_verifier/INDVerifierTestConfig - test_pfdtane/PFDTaneValidationParams - test_tane_afd_measures/TaneValidationParams - /PdepSelfValidationParams - test_typed_column_data/TypeParsingParams

Set libraries download in alphabetical order. Bump actions/checkout to v4. Change run_tests matrix to more readable. Add env field to avoid code duplications inside job. resolves Desbordante#512

Add `export CC=...`, because otherwise CMake fails to locate a tool for scanning C++ module dependencies (no matter we don't use modules): emscripten-core/emscripten#22305

p-senichenkov force-pushed the apple-clang-buildability branch from dac18fc to 2108d8a Compare January 3, 2025 11:30

p-senichenkov marked this pull request as ready for review January 3, 2025 11:31

Vdaleke self-requested a review January 3, 2025 14:54

Vdaleke requested changes Jan 4, 2025

View reviewed changes

Vdaleke requested changes Jan 5, 2025

View reviewed changes

p-senichenkov force-pushed the apple-clang-buildability branch from be21b26 to eb10e2e Compare January 6, 2025 13:05

Vdaleke requested changes Jan 6, 2025

View reviewed changes

github-actions bot reviewed Jan 6, 2025

View reviewed changes

src/core/util/bitset_extensions.h Show resolved Hide resolved

p-senichenkov force-pushed the apple-clang-buildability branch from 6fc2ab3 to 1bd7606 Compare January 6, 2025 20:37

github-actions bot reviewed Jan 7, 2025

View reviewed changes

src/core/algorithms/md/hymd/md_lhs.h Outdated Show resolved Hide resolved

src/core/algorithms/md/hymd/md_lhs.h Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 7, 2025

View reviewed changes

src/core/algorithms/md/hymd/md_lhs.h Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 7, 2025

View reviewed changes

src/core/algorithms/dd/split/split.cpp Outdated Show resolved Hide resolved

p-senichenkov force-pushed the apple-clang-buildability branch 2 times, most recently from 34a9520 to 859b5c0 Compare January 8, 2025 15:24

p-senichenkov requested a review from Vdaleke January 8, 2025 21:11

Vdaleke requested changes Jan 8, 2025

View reviewed changes

p-senichenkov changed the title ~~Add ability to build with Apple Clang~~ Add ability to build with Clang Jan 9, 2025

Vdaleke approved these changes Jan 12, 2025

View reviewed changes

Vdaleke force-pushed the apple-clang-buildability branch from 3096622 to 1b9b4af Compare January 12, 2025 19:48

p-senichenkov force-pushed the apple-clang-buildability branch from 1b9b4af to a39e86e Compare January 13, 2025 18:22

p-senichenkov mentioned this pull request Jan 14, 2025

FastADC implementation #470

Merged

p-senichenkov force-pushed the apple-clang-buildability branch from 20ad501 to 4750d2b Compare January 15, 2025 18:36

Vdaleke force-pushed the apple-clang-buildability branch from 4750d2b to 84c6dc9 Compare January 25, 2025 16:21

p-senichenkov mentioned this pull request Jan 28, 2025

Add macOS pip-packages. Add GCC-macOS configuration to CI. #513

Open

Vdaleke force-pushed the apple-clang-buildability branch from 33461cf to 9e563fc Compare January 29, 2025 11:52

p-senichenkov force-pushed the apple-clang-buildability branch 3 times, most recently from a9dff75 to be00164 Compare January 30, 2025 09:46

p-senichenkov force-pushed the apple-clang-buildability branch from be00164 to d6d0eeb Compare January 30, 2025 10:37

Vdaleke force-pushed the apple-clang-buildability branch from d6d0eeb to 50b2ba3 Compare January 30, 2025 14:10

p-senichenkov force-pushed the apple-clang-buildability branch from 50b2ba3 to e795576 Compare January 30, 2025 14:45

p-senichenkov and others added 8 commits January 30, 2025 17:52

Update "build" section of README

ea73eac

Add information about other compilers in README. Extra build instructions (Linux & Clang, macOS & LLVM CLang, macOS & GCC) are now moved to wiki

Add Clang tests to CI

28288e5

Add tests for Clang on Linux, LLVM Clang on macOS and Apple Clang on macOS. Also, bump boost version used in CI, as 1.8.10 produces an error on macOS with LLVM Clang. Here it's said that boost 1.85.0 works fine: bambulab/BambuStudio#3957

Refactor CI

8c9450c

Set libraries download in alphabetical order. Bump actions/checkout to v4. Change run_tests matrix to more readable. Add env field to avoid code duplications inside job. resolves Desbordante#512

Add export CC=... in LLVM Clang python-bindings tests

18d0748

Add `export CC=...`, because otherwise CMake fails to locate a tool for scanning C++ module dependencies (no matter we don't use modules): emscripten-core/emscripten#22305

p-senichenkov force-pushed the apple-clang-buildability branch from e795576 to 18d0748 Compare January 30, 2025 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to build with Clang #507

Add ability to build with Clang #507

p-senichenkov commented Dec 23, 2024 •

edited

Loading

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 4, 2025 •

edited

Loading

p-senichenkov commented Jan 4, 2025 •

edited

Loading

Vdaleke commented Jan 4, 2025

p-senichenkov commented Jan 4, 2025

Vdaleke commented Jan 4, 2025

Vdaleke left a comment •

edited

Loading

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 6, 2025 •

edited

Loading

Vdaleke commented Jan 6, 2025 •

edited

Loading

p-senichenkov commented Jan 6, 2025

github-actions bot left a comment

Vdaleke commented Jan 6, 2025

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 12, 2025 •

edited

Loading

Vdaleke commented Jan 30, 2025

p-senichenkov commented Jan 30, 2025

Vdaleke commented Jan 30, 2025

p-senichenkov commented Jan 30, 2025

Vdaleke commented Jan 30, 2025

Add ability to build with Clang #507

Are you sure you want to change the base?

Add ability to build with Clang #507

Conversation

p-senichenkov commented Dec 23, 2024 • edited Loading

Vdaleke left a comment • edited Loading

Choose a reason for hiding this comment

Vdaleke commented Jan 4, 2025 • edited Loading

p-senichenkov commented Jan 4, 2025 • edited Loading

Vdaleke commented Jan 4, 2025

p-senichenkov commented Jan 4, 2025

Vdaleke commented Jan 4, 2025

Vdaleke left a comment • edited Loading

Choose a reason for hiding this comment

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 6, 2025 • edited Loading

Vdaleke commented Jan 6, 2025 • edited Loading

p-senichenkov commented Jan 6, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

Vdaleke commented Jan 6, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Vdaleke left a comment • edited Loading

Choose a reason for hiding this comment

Vdaleke commented Jan 12, 2025 • edited Loading

Vdaleke commented Jan 30, 2025

p-senichenkov commented Jan 30, 2025

Vdaleke commented Jan 30, 2025

p-senichenkov commented Jan 30, 2025

Vdaleke commented Jan 30, 2025

p-senichenkov commented Dec 23, 2024 •

edited

Loading

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 4, 2025 •

edited

Loading

p-senichenkov commented Jan 4, 2025 •

edited

Loading

Vdaleke left a comment •

edited

Loading

p-senichenkov commented Jan 6, 2025 •

edited

Loading

Vdaleke commented Jan 6, 2025 •

edited

Loading

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 12, 2025 •

edited

Loading