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

max(a, b) template function error #23

Open
andan42 opened this issue Dec 5, 2022 · 3 comments
Open

max(a, b) template function error #23

andan42 opened this issue Dec 5, 2022 · 3 comments

Comments

@andan42
Copy link

andan42 commented Dec 5, 2022

./src/plot/plot_lib.hpp:3915:11: error: no matching function for call to 'max'
          utils::max(1l, rct.p2.y/cell_rows + (rct.p2.y%cell_rows != 0)) }
          ^~~~~~~~~~
./src/plot/plot_lib.hpp:59:24: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long' vs. 'int')
    inline constexpr T max(T a, T b) {
                       ^

Identical error in a few more places.

Fixed by adding this to utils:

template<typename T, typename _T>
    inline constexpr T max(T a, _T b) {
        return (a >= b) ? a : b;
    }

Probably should do this to min() too.

clang++-3.9
-g -MMD -MP -std=c++14

@fbbdev
Copy link
Owner

fbbdev commented Dec 5, 2022

Hello,

Thank you for reporting this. You're not the first to notice this issue, it should be related to pull request #19.

Unfortunately, the fix you suggest is not the right one. Max and min take only one template parameter for a reason, i.e. to suppress common type deduction behavior and ensure the returned type is exactly the same as the type of both arguments.

Your fix instead makes max return the type of the first argument, which is not the desired behavior.

As I noted in #19, the correct fix is to replace all constants like 0l, 1l, ... with Coord(0), Coord(1), ... at all offending locations (Coord should be the type of the other argument if I'm not mistaken).

The issue is due to an oversight on my part, I chose a lazy approach and failed to notice that it wouldn't work on some platforms.

Unfortunately I'm very busy and I haven't been able to find some time to fix this myself. I am ready to accept a pull request implementing the solution I suggested. I can't accept different solutions for the reason I explained above.

Kind regards,
Fabio

@fbbdev
Copy link
Owner

fbbdev commented Dec 5, 2022

By the way, I'm curious as to why you are compiling with clang 3.9. This is if I'm not wrong a very old version. If you are doing this just because it's the version specified in the README, please note that it was written long ago and I would now recommend more recent versions of the compiler. The library should be compliant with the ISO-C++14 standard, hence compatible with recent versions of clang.

@andan42
Copy link
Author

andan42 commented Dec 6, 2022

First off I'll have to say I'm not experienced with programming.
I'm compiling on a Raspberry Pi that for some reason I can't get to update packages properly, that's my mistake. I originally had the issue when compiling with gcc (not so old version I think) but the error text gcc gives is too hard for me to understand so I switched to clang.
I'm sorry if I wasted your time, pull request #19 is exactly the same issue.
I'll see if I have the time to properly make the changes you suggested but, since I'm just a hobbyist, the fix I used is fine for me. I was just leaving this message because I didn't realize this was a known issue.
Also, I have almost no idea how github works, let me know if I have to close this, because I am seriously clueless. Sorry again.

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

No branches or pull requests

2 participants