sys: util: Add SIGN, gcd and lcm

[ngphibang]

Add some helpers to util.h

• SIGN macro
• GCD (Greatest Common Divisor)
• LCM (Least Common Multiple)

which are helpul for example, used in #98514

[peter-mitsis]

This statement is not quite true. If both a and b are INT_MIN, then the result will be INT_MIN -- a negative value.

[ngphibang]

By mathematical definition, the greatest common divisor (GCD) of two integers is always a non-negative integer. But you are right, the old implementation didn't treat this edge case when both a and b are INT_MIN. We fixed this in the current push. Thanks !

[avolmat-st]

@ngphibang I've added a PR in the hal_stm32 (zephyrproject-rtos/hal_stm32#327) in order to avoid the duplication of SIGN macro. Since this is part of the hal_stm32 repo, you'll need to update the west.yaml in this PR in order to have twister tests working fine by using the hal_stm32 PR.

You can do that by adding a commit in this PR to update the manifest.
While the hal_stm32 PR isn't yet merged could you point to: pull/327/head as revision of hal_stm32 ?

aka below modification in the west.yml

diff --git a/west.yml b/west.yml
index b8845b415dc..32d7d1832b9 100644
--- a/west.yml
+++ b/west.yml
@@ -250,7 +250,7 @@ manifest:
       groups:
         - hal
     - name: hal_stm32
-      revision: 93e7944879c99bc5e0bb6371f9d2cb5d9a9f3482
+      revision: pull/327/head
       path: modules/hal/stm32
       groups:
         - hal

Once the PR in hal_stm32 will be merged you'll be able to update again the manifest to point to the new revision of the hal_stm32.

I was first thinking about creating a PR myself for the west.yml to update the hal_stm32 revision as I usually do, but I am afraid that our PR will depend on each others so it seems easier if your PR has the west.yml directly in it.
@erwango, is that ok this way ?

[josuah]

Good idea to split dependencies out of the main PR, which will hopefully make the main PR quicker to merge.

[pdgendt]

For this PR, you might want to get some inspiration from #96478 ?

[fabiobaltieri]

Yeah prob a good idea to do single evaluation macros whenever possible, for generics, this needs to wait for #97061

[trunghieulenxp]

Yeah prob a good idea to do single evaluation macros whenever possible, for generics, this needs to wait for #97061

Thanks, If I understand well, should the macro LCM be implemented like this:

#define LCM(a, b) \
({ \
    __typeof__(a) _a = (a); \
    __typeof__(b) _b = (b); \
    _Generic((a), \
        int8_t : lcm_s, \
        int16_t : lcm_s, \
        int32_t : lcm_s, \
        uint8_t : lcm_u, \
        uint16_t : lcm_u, \
        uint32_t : lcm_u)(_a, _b); \
})

so that a and b is evaluated once ?

Thanks.

[fabiobaltieri]

That's right, I'd make it lowercase in that case since then it can only be used in functions and then the multiple evaluation version can still be there in uppercase. That's how we did min/max and how Linux does it too.

[trunghieulenxp]

That's right, I'd make it lowercase in that case since then it can only be used in functions and then the multiple evaluation version can still be there in uppercase. That's how we did min/max and how Linux does it too.

Thanks. However, in this case, I don’t think we should implement the version with multiple evaluation because the functions lcm_s, gcd_s, lcm_u, and gcd_u will be declared as ALWAYS_INLINE. After the last comment in 96478#issuecomment-3368956442 lcm and gcd macros will always be single-evaluation macros.

The most challenging part here is the acceptance of _Generic in Zephyr. Nevertheless, I’ve noticed that _Generic is already used in the C_PRINTF() macros, so at this point I don’t think this should have any problem.

As lcm and gcd are single evaluated macro, so these macros be in lower case only ? @josuah @fabiobaltieri

Thanks.

[fabiobaltieri]

The most challenging part here is the acceptance of _Generic in Zephyr. Nevertheless, I’ve noticed that _Generic is already used in the C_PRINTF() macros, so at this point I don’t think this should have any problem.

Yeah but I think that function is optional? Not too sure, anyway mandatory C17 support has been discussed and voted by TSC last week and it looks like it was going to get in, will keep you posted.

As lcm and gcd are single evaluated macro, so these macros be in lower case only ? @josuah @fabiobaltieri

Yeah, from the min/max discussion, the reasoning about doing the macro in lowercase is that the way it's implemented it effectively behave as a function, should probably write it down in the coding style somewhere because I think it makes a lot of sense. Like you can't use in a BUILD_ASSERT or anything that must be evaluated statically.

Doing only the single evaluation I think is fine, avoid having people picking the wrong implementation unintentionally, if the macro one is needed it can always be introduced at a later stage.

[github-actions]

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_stm32	zephyrproject-rtos/hal_stm32@286dd28 (main)	zephyrproject-rtos/hal_stm32#327	zephyrproject-rtos/hal_stm32#327/files

⛔ DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[ngphibang]

I was first thinking about creating a PR myself for the west.yml to update the hal_stm32 revision as I usually do, but I am afraid that our PR will depend on each others so it seems easier if your PR has the west.yml directly in it.
@erwango, is that ok this way ?

Thanks @avolmat-st. I added the west.yml revision commit into this PR. However, this PR may take some time to be merged, so does it block your PR ?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[avolmat-st]

I was first thinking about creating a PR myself for the west.yml to update the hal_stm32 revision as I usually do, but I am afraid that our PR will depend on each others so it seems easier if your PR has the west.yml directly in it.
@erwango, is that ok this way ?

Thanks @avolmat-st. I added the west.yml revision commit into this PR. However, this PR may take some time to be merged, so does it block your PR ?

No issue @ngphibang. It is not blocking the hal_stm32 PR which has actually already been merged and does not block other hal_stm32 related PRs.
You can update your PR with the proper sha1 for hal_stm32 instead of the PR link. It is merged in 2c18f2b49d66d23cabfbd20dd7dbbaef8ee9520b.