Ultimaker / CuraEngine

Powerful, fast and robust engine for converting 3D models into g-code instructions for 3D printers. It is part of the larger open source project Cura.
https://ultimaker.com/en/products/cura-software
GNU Affero General Public License v3.0
1.67k stars 880 forks source link

Replace size_t format string in InfillTest.cpp #1898

Closed onitake closed 1 year ago

onitake commented 1 year ago

Description

The format string in tests/InfillTest.cpp is not correct for the (last) size_t argument, causing test failure on 32-bit architectures. This PR replaces the %lld with the correct C99 format string for size_t: %zu.

It's expected that this will work with every current C standard library used for CuraEngine. If this is not the case and C90 C library compatibility is desired, a more conservative format string can be used, such as %llu. This was tested successfully on i686 with gcc 12/glibc 2.37, even though it's not 100% correct for a 32-bit size_t.

Fixes: #1897

Type of change

How Has This Been Tested?

Test Configuration:

Checklist:

onitake commented 1 year ago

Hi @onitake I'm just curious. I noticed you asking some specific Linux questions libCharon now a PR for 32bit hardware. I'm guessing you're a package maintainer for one of the distro's.

That is correct, I'm one of the maintainers of the Debian packages: https://tracker.debian.org/pkg/cura-engine Maintenance got a bit stuck lately due to lack of time and the switch from CMake to Conan, but we're intent to get at least 5.0 maintained in vanilla Debian.

Since Debian still includes some 32-bit release architectures, we need to ensure compatibility if possible.

We recently added fmt https://fmt.dev/latest/index.html as one of the dependencies. Since the TODO above states that this should be changed anyhow once we use fmt I would prefer it if this PR uses the fmt lib.

Ok, I can look into that. I don't know that fmt library, but it sounds straight-forward.

jellespijker commented 1 year ago

basically the same syntax as the Python format strings

onitake commented 1 year ago

Ok, so I replaced makeName with fmt::format. The format string explicitly uses decimal format {:d}, so I dropped the static_casts - except for the EFillMethod type.

It looks like fmtlib won't automatically cast enum classes to int starting from v7, to be in line with C++20 semantics. You either need to add a custom formatter (as in https://fmt.dev/latest/api.html#formatting-user-defined-types) or cast it to the underlying type yourself. See https://github.com/fmtlib/fmt/issues/2704 for an example and the reasoning.