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

Eclipse codan finds interger_quotient ambiguous #219

Open
pirgia opened this issue Feb 24, 2024 · 4 comments
Open

Eclipse codan finds interger_quotient ambiguous #219

pirgia opened this issue Feb 24, 2024 · 4 comments
Labels
needs root cause Cases where we don't fully understand what is causing the problem

Comments

@pirgia
Copy link

pirgia commented Feb 24, 2024

Well, I am a new user of the library and, obviously as a newbie, I am facing issues in its use.
I already solved many, but this one I am not.
I am developing for an STM32 microcontroller in the CUBEIDE environment (basically eclipse).
I have all of the CODAN code checks turned on and when performing the integer division between two frequencies in Hertz stored in 32-bits the editor warns against an ambiguous call.

This is the call
uint32_t const prescaler = integer_quotient(timer_clock_frequency, au::hertz(1000000UL));

This is the problem I get:
Problem description: 'integer_quotient' is ambiguous ' Candidates are:
- au::Quantityau::pow,au::Quantity integer_quotient(au::Quantity, au::Quantity)
- au::Quantity<au::hertz,au::quantity<au::pow,unsigned long int>> integer_quotient(au::Quantity, au::Quantity)

I believe some template has a "auto" return type but is called with a partial specialization that confuses the code analyzer, while the compiler, instead does not provide any issue.
Thanks for your attention.

@chiphogg
Copy link
Contributor

Thanks for your interest in the library, and for taking the time to file an issue! I'm also of course always delighted to see the library find use among embedded users.

I'm a little confused by the problem description. The formatting has swallowed a couple of the < and > symbols, but I can recover them by viewing the source of your post, by clicking "Edit" and then cancelling. Here they are:

au::Quantity<au::pow,au::Quantity> integer_quotient(au::Quantity, au::Quantity)
au::Quantity<au::hertz,au::quantity<au::pow,unsigned long int>> integer_quotient(au::Quantity, au::Quantity)

Here are a couple things that I find confusing.

  • au::Quantity<au::pow, au::Quantity> isn't something I recognize from the library: I don't know of any au:::pow that could appear in this way, and it's very strange for Quantity to be a template argument of Quantity.
  • It's strange to see both uppercase Quantity and lowercase quantity in the second line.

Are these simply copy-pasted from your editor and/or tooling? I admit I'm a bit stumped as to where they could have come from! Do you have a little more code context you could provide? (For example, the definition of timer_clock_frequency?)

Note, too, that you can try reproducing the problem in Compiler Explorer. If you're able to find a matching compiler and reproduce the error, that would help me fix it.

@chiphogg
Copy link
Contributor

In any case, you might be able to work around the problem and satisfy your use case directly with something like the following:

uint32_t const prescaler = timer_clock_frequency.coerce_in(au::mega(au::hertz));

This should do the same thing under the hood, as you can see from this Compiler Explorer link: it's just a truncating integer divide by 1,000,000. The reason we say .coerce_in instead of .in is because Au guards against truncation by default. Saying .coerce_in will override this safety check, because the truncating division is desired.

As a bonus, this might also make your intent a little clearer at the callsite.

@pirgia
Copy link
Author

pirgia commented Feb 25, 2024

First of all, let me state it very clearly:
- The library compiles and runs fine
and the compiler is a GCC-derived one.

What is causing issues is the code analyzer integrated into Eclipse IDE (CODAN) - here is the link if you need to get a reference: codan -

I think of the code analyzer as a smart peer reviewer on my side, warning me about unused variables, wrong types etc. and I have all the checks turned on.
The code analyzer has defects, and I try to find workarounds for them. The last stand against false positives is to suppress the warning.
Unfortunately, the units library is giving the code analyzer hard work and it - the code analyzer - responds with a lot of false positives.

timer_clock_frequency has a type of au::Quantity<au::Hertz, uint32_t>, same as au::QuantityU32<au::Hertz>

Here some examples:

  • I started coercing timer_clock_frequency in MHz by using
    uint32_t const prescaler = timer_clock_frequency.coerce_in(au::mega(au::hertz));

and this what I get (pasted from the editor - no formatting this time)
Problem description: Invalid arguments ' Candidates are: 8 2 201 2
au::Quantity::in au::Quantity::in 1 8321 9 0 78 #65536 0 0 coerce_in(#10000) 8 2
201 2 au::Quantity::in au::Quantity::in 1 #65536 0 78 #65537 0 0 coerce_in
(#10001) '

another example

au::QuantityU32<au::Hertz> a_frequency = au::hertz(1000); // This is fine

au::QuantityU32<au::Hertz> a_frequency = au::kilo(au::hertz)(1); // This is not

au::QuantityU32<au::Hertz> a_frequency;
a_frequency = au::kilo(au::hertz)(1); // this is fine again

Here is the original warning message from the editor - no formatting:
Problem description: 'integer_quotient' is ambiguous ' Candidates are:
au::Quantityau::pow,au::Quantity integer_quotient(au::Quantity,
au::Quantity) au::Quantity<au::hertz,au::quantity<au::pow,unsigned long int>>
integer_quotient(au::Quantity, au::Quantity) '

Again, the library is fine, only I don't like to fill the source code with // @suppress comments. That's it.

Thanks again for the attention

@chiphogg
Copy link
Contributor

First of all, let me state it very clearly: - The library compiles and runs fine and the compiler is a GCC-derived one.

What is causing issues is the code analyzer integrated into Eclipse IDE (CODAN) - here is the link if you need to get a reference: codan -

I think of the code analyzer as a smart peer reviewer on my side, warning me about unused variables, wrong types etc. and I have all the checks turned on. The code analyzer has defects, and I try to find workarounds for them. The last stand against false positives is to suppress the warning. Unfortunately, the units library is giving the code analyzer hard work and it - the code analyzer - responds with a lot of false positives.

timer_clock_frequency has a type of au::Quantity<au::Hertz, uint32_t>, same as au::QuantityU32<au::Hertz>

Here some examples:

  • I started coercing timer_clock_frequency in MHz by using
    uint32_t const prescaler = timer_clock_frequency.coerce_in(au::mega(au::hertz));

and this what I get (pasted from the editor - no formatting this time) Problem description: Invalid arguments ' Candidates are: 8 2 201 2 au::Quantity::in au::Quantity::in 1 8321 9 0 78 #65536 0 0 coerce_in(#10000) 8 2 201 2 au::Quantity::in au::Quantity::in 1 #65536 0 78 #65537 0 0 coerce_in (#10001) '

Interesting --- I don't really know what all those numbers mean.

another example

au::QuantityU32<au::Hertz> a_frequency = au::hertz(1000); // This is fine

au::QuantityU32<au::Hertz> a_frequency = au::kilo(au::hertz)(1); // This is not

au::QuantityU32<au::Hertz> a_frequency;
a_frequency = au::kilo(au::hertz)(1); // this is fine again

Here is the original warning message from the editor - no formatting: Problem description: 'integer_quotient' is ambiguous ' Candidates are: au::Quantityau::pow,au::Quantity integer_quotient(au::Quantity, au::Quantity) au::Quantity<au::hertz,au::quantity<au::pow,unsigned long int>> integer_quotient(au::Quantity, au::Quantity) '

How strange.

Again, the library is fine, only I don't like to fill the source code with // @suppress comments. That's it.

Thanks again for the attention

Thanks! That makes sense. I'm a big fan of static analysis, and also not a big fan of having to insert a bunch of // @suppress comments everywhere.

Unfortunately, I don't expect we'll be able to dig into this any time soon. I'm also not sure what we would be able to do about this even if we could. Maybe the warnings from codan are indicating real opportunities to improve our coding style? Then again, maybe they're just bugs in codan. Units library implementations tend to push C++ templates far beyond what is usual, and sometimes that can expose overly simplistic assumptions in tooling.

At any rate, I'm glad that the library itself is working fine!

@chiphogg chiphogg added the needs root cause Cases where we don't fully understand what is causing the problem label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs root cause Cases where we don't fully understand what is causing the problem
Projects
None yet
Development

No branches or pull requests

2 participants