fix: remove redundancy with constexpr variables

0xBLAHAJ commented 2 months ago

constexpr variables are evaluated at compile time, rendering the inline and const keywords useless since they don't persist in memory this is only beneficial for file size as constexpr takes priority over other keywords (so probably the lowest priority pull request yet)

Code change checklist

[x] I have ensured that all methods and functions are fully documented using doxygen style comments.
[x] My code follows the coding style guide.
[x] I tested that my change works before raising the PR.
[x] I have ensured that I did not break any existing API calls.
[x] I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

CLAassistant commented 2 months ago

All committers have signed the CLA.

netlify[bot] commented 2 months ago

Deploy Preview for dpp-dev ready!

Name	Link
Latest commit	eb3fd397c2db322ad0b55d8a5e6af9b744d33100
Latest deploy log	https://app.netlify.com/sites/dpp-dev/deploys/66422f69e9162800082ad240
Deploy Preview	https://deploy-preview-1149--dpp-dev.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Mishura4 commented 2 months ago

Constexpr variables are not implicitly inline, inline is not redundant on variables. Constexpr functions are, but not constexpr variables.

Try including the header in two translation units without the inline, you'll get multiple definition errors.

Additionally, this PR does not do much. This doesn't fix any warning or errors, and doesn't add any feature, so I'm not sure what the purpose is.

braindigitalis commented 2 months ago

These changes wouldnt be made here anyway; the unicode_emojis.h is auto generated so any changes made directly to the file will be dropped. Perhaps we should have an action that auto identifies and flags changes to auto-generated files where there hasn't also been a change to the generator script that creates it?

Jaskowicz1 commented 2 months ago

These changes wouldnt be made here anyway; the unicode_emojis.h is auto generated so any changes made directly to the file will be dropped. Perhaps we should have an action that auto identifies and flags changes to auto-generated files where there hasn't also been a change to the generator script that creates it?

Sounds like a good idea!

0xBLAHAJ commented 2 months ago

Constexpr variables are not implicitly inline, inline is not redundant on variables. Constexpr functions are, but not constexpr variables.

constexpr variables aren't implicitly inline because they're not "true" variables but rather replaced by the value they hold at compile time a variable declared inline (without constexpr) will create a ~~subroutine~~ label that gets the value it holds in ASM, inline constexpr is optimized away to constexpr because it takes priority over other keywords e.g.: (using gcc 14.1 at -O3) (for reference, inline constexpr does not create the get subroutine but rather replace the invokation with the value)

Try including the header in two translation units without the inline, you'll get multiple definition errors.

#pragma once takes care of this, it does the same as

#ifndef NAME_H
#define NAME_H
// ... rest of the header file
#endif

and therefore nullifies multiple definition errors

Mishura4 commented 2 months ago

No, constexpr variables are variables, you can take their address. What ASM the compiler generates has nothing to do with this and especially with optimizations enabled.

As i said, try including the header without the inline in two translation units. Header guards also have nothing to do with this.

braindigitalis commented 2 months ago

but have you tried it, in dpp?

also, did you read the bit about this entire file being auto generated? Running cmake with all the correct dev depenencies will overwrite your changes.

0xBLAHAJ commented 2 months ago

No, constexpr variables are variables, you can take their address. What ASM the compiler generates has nothing to do with this and especially with optimizations enabled.

Not sure what you mean by 'what ASM the compiler generates has nothing to do with this', this is about how compilers handle inline, here it's redundant because of the constexpr keyword constexpr variables can have a place in memory but this isn't always the case, it actually is only the case if the variable in question can not always be evaluated at compile time (e.g. if you try to reference it's address, it obviously needs to place it somewhere in runtime memory thanks to ASLR) devenv_J05JOpoIWl (compiled with -Od on MSVC)

As i said, try including the header without the inline in two translation units. Header guards also have nothing to do with this.

You mentioned multiple definition errors which are non-existent on constexpr variables constexpr variables are also just not source of multiple definitions

I do agree i was wrong on the usage of #pragma once though, an error will be thrown on non-constexpr variables

but have you tried it, in dpp?

compiled w/ x64-clang-debug on a win32 machine, did not break anything because it simply does not change how clang (or any compiler for that matter) reads the file

Mishura4 commented 2 months ago

Not sure what you mean by 'what ASM the compiler generates has nothing to do with this', this is about how compilers handle inline, here it's redundant because of the constexpr keyword

No, it's not. inline in c++ is about allowing the linker to merge multiple identical definitions into one, it's not about the compiler replacing a variable indirection with its value. inline is not redundant because constexpr variables are not inline by default.

The compiler or linker will do that replacement when it can, inline or not, it has nothing to do with this.

brainboxdotcc / DPP