clyne
/
constexpr-to-string
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Labels Milestones

Removing magic constant from template param check #3

Merged
SecMeant merged 3 commits from digits_array into master 4 years ago
Conversation 5 Commits 3 Files Changed 2
SecMeant commented 4 years ago (Migrated from github.com)

Storing digits in variable, thus removing magic constant from template param check.

Storing digits in variable, thus removing magic constant from template param check.
tcsullivan commented 4 years ago (Migrated from github.com)

This would be a nice improvement; however, it's leaving us with digits and digit_count in the global scope, which could mess up other code. One solution to that would be to put everything in a namespace, though I haven't done that yet (not sure what to name it). Open to any other solutions if you have them.

This would be a nice improvement; however, it's leaving us with `digits` and `digit_count` in the global scope, which could mess up other code. One solution to that would be to put everything in a namespace, though I haven't done that yet (not sure what to name it). Open to any other solutions if you have them.
Neargye commented 4 years ago (Migrated from github.com)

@tcsullivan namespace cits // constexpr_integral_to_string ?

@tcsullivan `namespace cits // constexpr_integral_to_string` ?
Neargye (Migrated from github.com) requested changes 4 years ago
to_string.hpp
@ -11,1 +11,4 @@
namespace constexpr_to_string {
constexpr char digits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
Neargye (Migrated from github.com) commented 4 years ago

inline constexpr

`inline constexpr `
to_string.hpp
@ -12,0 +12,4 @@
namespace constexpr_to_string {
constexpr char digits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
constexpr auto digit_count = sizeof(digits) / sizeof(digits[0]);
Neargye (Migrated from github.com) commented 4 years ago

also inline constexpr

also `inline constexpr`
m-peko (Migrated from github.com) reviewed 4 years ago
to_string.hpp
@ -11,1 +11,4 @@
namespace constexpr_to_string {
constexpr char digits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
m-peko (Migrated from github.com) commented 4 years ago

@Neargye why inline constexpr when constexpr itself is implicitly inline?

@Neargye why `inline constexpr` when `constexpr` itself is implicitly inline?
Neargye (Migrated from github.com) reviewed 4 years ago
to_string.hpp
@ -11,1 +11,4 @@
namespace constexpr_to_string {
constexpr char digits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
Neargye (Migrated from github.com) commented 4 years ago

​constexpr specifier implies  inline for static data members as well as functions, but not for global variable.
https://en.cppreference.com/w/cpp/language/inline

​constexpr specifier implies  inline for static data members as well as functions, but not for global variable. https://en.cppreference.com/w/cpp/language/inline
SecMeant commented 4 years ago (Migrated from github.com)

Now, when I think about it, maybe we should use something like

namespace to_string_ { ... }

Since its only purpose is to only hide digits symbols.
@tcsullivan

Now, when I think about it, maybe we should use something like namespace to_string_ { ... } Since its only purpose is to only hide digits symbols. @tcsullivan
tcsullivan commented 4 years ago (Migrated from github.com)

@SecMeant The constexpr_to_string namespace is honestly probably enough. I just realized that the final to_string declaration is outside of the namespace, so this isn't actually affecting how to_string is called (something I was concerned about).

I'm good to merge this PR as it is now, if you are.

@SecMeant The `constexpr_to_string` namespace is honestly probably enough. I just realized that the final `to_string` declaration is outside of the namespace, so this isn't actually affecting how `to_string` is called (something I was concerned about). I'm good to merge this PR as it is now, if you are.
SecMeant commented 4 years ago (Migrated from github.com)

Sure

Sure

Reviewers

Neargye
The pull request has been merged as 6710f1e699.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b digits_array master
git pull origin digits_array

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff digits_array
git push origin master
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Neargye
Labels
Apply labels
Clear labels   
bug

Something isn't working   
documentation

Improvements or additions to documentation   
duplicate

This issue or pull request already exists   
enhancement

New feature or request   
good first issue

Good for newcomers   
help wanted

Extra attention is needed   
invalid

This doesn't seem right   
question

Further information is requested   
wontfix

This will not be worked on
No Label
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: clyne/constexpr-to-string#3
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'digits_array'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes