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

fix: remove redundancy with constexpr variables #1149

Closed
wants to merge 1 commit into from
Closed

fix: remove redundancy with constexpr variables #1149

wants to merge 1 commit into from

Conversation

0xBLAHAJ
Copy link

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

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

constexpr variables are evaluated at compile time and replace each of their invokation with it's value before compilation to ASM
both inline and const as keywords are redundant/do not do anything to constexpr variables
@CLAassistant
Copy link

CLAassistant commented May 13, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented May 13, 2024

Deploy Preview for dpp-dev ready!

Name Link
🔨 Latest commit eb3fd39
🔍 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...

QR Code

Use your smartphone camera to open QR code link.

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

@github-actions github-actions bot added documentation Improvements or additions to documentation code Improvements or additions to code. labels May 13, 2024
@Mishura4
Copy link
Member

Mishura4 commented May 13, 2024

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.

@Mishura4 Mishura4 closed this May 13, 2024
@braindigitalis
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

0xBLAHAJ commented May 13, 2024

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)
image
image
(for reference, inline constexpr does not create the get subroutine but rather replace the invokation with the value)
image

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
Copy link
Member

Mishura4 commented May 13, 2024

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
Copy link
Contributor

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
Copy link
Author

0xBLAHAJ commented May 13, 2024

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)
image
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
image
constexpr variables are also just not source of multiple definitions
image

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
Copy link
Member

Mishura4 commented May 13, 2024

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 brainboxdotcc locked as resolved and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
code Improvements or additions to code. documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants