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

Replace macro usage for constants #2655

Open
Lestropie opened this issue Jun 15, 2023 · 2 comments
Open

Replace macro usage for constants #2655

Lestropie opened this issue Jun 15, 2023 · 2 comments
Labels

Comments

@Lestropie
Copy link
Member

Bears some relation to #2111, in that it might be nice to address old code structure as part of a general cleanup potentially in #2585.

Preprocessor macros are regularly used to define default values for parameters, often in cases where this value may be controllable at the command-line and it is desired for the default value to be printed as part of the help string for that option. However these are not properly context-bound, and in some instances I think I've found duplicate macros purporting to serve the same purpose but used in different places.

These days I try to use eg. a constexpr in the relevant namespace; eg. 7ae2f62. But it could be worthwhile to do an audit and clean up all residual lazy macro usage.

@daljit46
Copy link
Member

daljit46 commented Jun 16, 2023

I agree that #define statements need to go, and constexpr probably can replace the vast majority of use cases. However, I'm not sure whether the c++17 branch is the right place for this change (as I believe this is tangential). More importantly, I think we should be careful at amassing a large amount of changes in one pull request because it will most likely cause subtle changes in behaviour and bugs that may be hard to detect and non-obvious at first sight.

@Lestropie
Copy link
Member Author

It might be the case that this and #2111 fit together as sort of "substitution of old code practises out of the back end". If there are some aspects of #2585 that end up being left out of the main C++17 transition due to not being strictly necessary, eg. something like adopting std::byte, that might then appropriately fall into this new category. But agree it is technically a different focus to "adoption of C++17".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants