Skip to content

new C++ style #12

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

Closed
wants to merge 3 commits into from
Closed

new C++ style #12

wants to merge 3 commits into from

Conversation

strega-nil
Copy link

No description provided.

@strega-nil
Copy link
Author

strega-nil commented Sep 18, 2017

@leios Could you make some comments or something? I know this is a major change, but I'd like to at least talk through it? perhaps make some edits to make it more palatable.

@june128
Copy link
Member

june128 commented Sep 18, 2017

Not a C++ guy, but why are you using std::uint64_t instead of int?

@Gathros
Copy link
Contributor

Gathros commented Sep 18, 2017

James asked for it.

@leios
Copy link
Member

leios commented Sep 18, 2017

Hey, sorry for the late reply here.

I had a discussion with some of the rest of the community on discord about this pull request (let me know if you want a link). A few things:

  1. I think you are right that the comment block at the start should go away if we display the code in the version we are currently displaying it; however, I'm trying to think of new ways to display the code right now. In the end, I like a good comment block at the start of the code. It might be annoying in this context, but it is useful and good programming practice (imo).
  2. Readability is more important than functionality for these examples, which is why I kinda prefer int to std::uint64_t and would like to stay away from auto wherever possible.
  3. Your discussion brought up a few questions that we needed to address about negative numbers and the greatest common divisor, which lead to a modification in several of the currently standing code examples to make them more correct (mostly just adding abs()). Thanks for that!
  4. I have no doubt that there will be algorithms in the future where using more C++ features will not only improve readability, but also the reliability of the code. I just think this was too simple of an example for this to help.

Anyway, sorry for the long response and let me know what you think! Thanks again for the help so far!

@strega-nil
Copy link
Author

@Gathros because int is often not the correct type. It's implementation defined how big it is, and I wrote this to be more similar to C++ you should actually use.

@strega-nil
Copy link
Author

strega-nil commented Sep 18, 2017

@leios Not using auto is... not very C++-y. This is a classic case where auto is the correct thing to use. It might seem "less readable", but... let me have someone else make my argument for me: https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/

imo, this C++ code should be as close to recommended style as possible.

(basically, it's not less readable, you just aren't used to that style.)

@Gathros
Copy link
Contributor

Gathros commented Sep 18, 2017

I know, I was answer the question of why change int std::uint64_t?.

@strega-nil
Copy link
Author

That is a point that I could change though, if you feel that using int is more... readable, let's say. I still think we should use auto in the bodies of the functions.

@Butt4cak3
Copy link
Contributor

Butt4cak3 commented Sep 18, 2017

It's not only about readability. There is no reason not to allow negative numbers for a gcd function.

gcd(-10, 8) = 2

If you take a look at the pull requests #14, #15 and #16 you will see that most if not all samples have been updated with support for negative numbers. I would recommend adding that patch to your C++ implementation and reverting the types of the parameters back to a signed integer type to keep everything uniform.

I won't comment on the use of auto. I'm not qualified to do that.

@strega-nil
Copy link
Author

see #17

@strega-nil strega-nil closed this Sep 19, 2017
@strega-nil strega-nil deleted the new-c++-style branch February 16, 2018 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants