search

tallbl0nde / NX-Activity-Log

Homebrew application for the Nintendo Switch which displays play activity with more accuracy
MIT License
417 stars 27 forks source link

Use CPP Standard PI value from C++20 rather than using from math.h #41

Closed eXhumer closed 4 years ago

eXhumer commented 4 years ago

https://github.com/tallbl0nde/NX-Activity-Log/blob/d0baba98c06e1dc04cb641c66bb5827d59df6231/source/ui/Theme.cpp#L33 https://github.com/tallbl0nde/NX-Activity-Log/blob/d0baba98c06e1dc04cb641c66bb5827d59df6231/source/ui/Theme.cpp#L34 https://github.com/tallbl0nde/NX-Activity-Log/blob/d0baba98c06e1dc04cb641c66bb5827d59df6231/source/ui/Theme.cpp#L35 https://github.com/tallbl0nde/NX-Activity-Log/blob/d0baba98c06e1dc04cb641c66bb5827d59df6231/source/ui/Theme.cpp#L36

4 of these lines seem to be referencing a pre-processor defined value M_PI, but that value seems to be undefined. Where exactly can I find the definition for it, as I can't seem to be able to find it anywhere in any of the project header files. I also tried searching the math.h/cmath headers for the define, but didn't find it there either.

tallbl0nde commented 4 years ago

I'll be honest I have no idea... when I first wrote this I just googled how to access the value of pi and I'd found this.

I'm pretty sure it's meant to be in one of the math headers (in this case math.h) but I've never actually searched for it. I've just used it and it compiles + works without issues now. I believe it's how you're meant to use pi in C++, but don't hold me to that!

eXhumer commented 4 years ago

I have a better suggestion that is more C++ friendly. Switch the project to use gnu++2a rather than gnu++17. It will allow us to use C++20 features such as std::numbers::pi which is defined in numbers.

tallbl0nde commented 4 years ago

I'm happy to do that, a quick Google shows that gnu++2a is experimental though. I have no idea what that means in terms of stability or anything, I'm guessing it shouldn't matter?

The other option would be to simply declare it in the file.

eXhumer commented 4 years ago

You are right about gnu++2a being experimental, I though it was out already as it was supposed to be out by May 2020. I guess you can switch it when it comes out. IMO, it is better to use a value defined in CPP Standard Library rather than relying on a POSIX extension.

ioistired commented 4 years ago

It's defined by SDL2.

tallbl0nde commented 4 years ago

Ah I didn't know, in that case I'd say it's fine to continue using M_PI as I'm not planning to move away from SDL2. I'm still open to hearing any other thoughts though.

eXhumer commented 4 years ago

Just an FYI to make my point even more valid, Atmosphere-NX uses C++20 currently for build. So, I would say using C++20 would be fine to use at this point, but I will leave that up to you.

tallbl0nde commented 4 years ago

In that case I'd say so! I'll change it over then.

ioistired commented 4 years ago

This is pretty braindead. Requiring the latest C++ version just for a constant that has not changed in millenia, when there's already one defined in a library that NX-AL already uses is just dumb. What does Atmosphere-NX's build configuration have to do with it? Unless you meant libnx? Even in that case, not everyone has a C++20 compiler, but they may have precompiled versions of the library installed via DevKitPro. What do you gain?

tallbl0nde commented 4 years ago

I'm not one for sticking on a certain version of something just because it does everything I need. If something newer is available and has no apparent drawbacks I don't see the issue of upgrading. The Atmosphere argument was just to inform me with regards to reliability.

I'm assuming since this uses the devkitpro toolchain to compile everyone who wants to compile this would have a C++20 compiler installed (as a part of dkp)? Or does dkp just 'wrap around' compilers on the system?

What do I gain? C++20 features but apart from that, not much. What do I lose? Not much that I can tell either.

ioistired commented 4 years ago

No. dkp, at least on Linux, uses your system compiler. So someone might have latest dkp libs installed but not the latest compiler.

eXhumer commented 4 years ago

No. dkp, at least on Linux, uses your system compiler. So someone might have latest dkp libs installed but not the latest compiler.

??? You do realize all homebrew related Nintendo Switch stuff requires a custom compiler (custom GNU Compiler to be more specific), a compiler which happens to provided by said DKP (devkitA64 package to be more specific). It definitely doesn't use system compiler. Do not talk nonsense if you have no idea on what you are taking about in the first place.

eXhumer commented 4 years ago

This is pretty braindead. Requiring the latest C++ version just for a constant that has not changed in millenia, when there's already one defined in a library that NX-AL already uses is just dumb. What does Atmosphere-NX's build configuration have to do with it? Unless you meant libnx? Even in that case, not everyone has a C++20 compiler, but they may have precompiled versions of the library installed via DevKitPro. What do you gain?

Your point is completely invalid in the sense that you have no idea on what you are talking about regarding anything related to building a C/C++ application. There is everything to lose and nothing to gain by not upgrading to the next gen language specification. Just because you use GitHub doesn't mean you know anything about how homebrew development works and should avoid talking in places you don't belong, like here for example.

eXhumer commented 4 years ago

And regarding me mentioning Atmosphere using C++20, it was to illustrate a point that the firmware for which we are building the application for is OK with using C++20, but somehow it is not OK for the homebrew application to C++20 standard and use a feature from that instead of relying on a definition you can't even show where it is defined.

eXhumer commented 4 years ago

Also regarding someone not having C++20 supported compiler, DKP currently provides 10.1.0 gcc, same version as GNU source which includes the standard, so I am not even sure how you came to that conclusion in the first place.

ioistired commented 4 years ago

My apologies about devkitA64. I was under a different impression about it. I do actually have some knowledge about building C/C++ applications, having worked with C for a couple of years. But homebrew is new to me. So I'm sorry for taking such a confident and dismissive tone.

As for the issue at hand, I am still against it, but I suppose it doesn't matter in the end. An issue about whether to use one definition of the same constant or another should not have attracted 15 comments. Regardless, if everyone can compile C++20 then it's whatever.

Hope you're doing well in this tough time.

tallbl0nde commented 4 years ago

I can definitely appreciate that changing the C++ version probably is a bit over the top for one little constant, however since everyone should be able to compile it and it hasn't caused any issues I figured it's fine (I wouldn't have done it otherwise). It does seem like there's a few handy features (such as string formatting) that I might look into and use too.

I'm new to most of this as well, this is one of my first proper projects outside of Uni assignments so I'm still learning a lot!

Hope you're both doing well also. Let's leave this issue here :P