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.

Sign up for GitHub

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

Jump to bottom

Pass methods arguments by const refs #161

Merged
merged 5 commits into from Feb 5, 2020
Merged

Pass methods arguments by const refs #161

merged 5 commits into from Feb 5, 2020

Conversation

Razdeep
Copy link
Contributor

@Razdeep Razdeep commented Feb 3, 2020 •
edited by coder3101

Description

Addresses Issue #151

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate)

Type of changes

  • Bug fix (changes which fix an issue)
  • New feature (changes which add functionality)
  • Documentation (changes which modify the documentation only)
  • Chore (changes which do not belong to any type above)
  • Style change (changes which do not affect the meaning of the code: code formatting, etc.)
  • Refactor (changes which affect the meaning of the code but neither fix a bug nor add a feature)
  • Performance improve (changes which improve performance)
  • Test (changes which add tests)
  • Build (Changes that affect the build system or external dependencies)
  • CI (changes to CI configuration files and scripts)
  • Revert (revert previous changes)

Checklist

  • I have read the CONTRIBUTING document.
  • I have tested these changes locally, and this fixes the bug/the new feature behaves as the expectation.
  • I have used clang-format-9 and .clang-format file in the root directory to format my codes.
  • The settings file in the old version can be used in the new version after this change.
  • These changes only fix a single bug/introduces a single feature. (Otherwise, open multiple Pull Requests instead, unless these bugs/features are closely related.)
  • The commit messages are clear and detailed. (Otherwise, use git reset and commit again, or use git rebase -i and git commit --amend to modify the commit messages.)
  • These changes don't remove an existing feature. (Otherwise, add an option to disable this feature instead, unless it's necessary to remove this feature.)

@caretaker-claire
Copy link

Hi Razdeep
Thank you for your contribution to cp-editor. I will request a review from one of the contributor. Once they approves and all CI checks passes, I will merge your Pull Request.

coder3101
coder3101 reviewed Feb 3, 2020
View reviewed changes
src/Core/Formatter.cpp Outdated
{
this->log = log;
updateBinary(clangFormatBinary);
updateStyle(clangFormatStyle);
Formatter::updateBinary(clangFormatBinary);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not fully qualify the name. It may confuse users to think that updateBinary is a static function.

coder3101
coder3101 reviewed Feb 3, 2020
View reviewed changes
src/Core/Formatter.cpp Outdated
updateBinary(clangFormatBinary);
updateStyle(clangFormatStyle);
Formatter::updateBinary(clangFormatBinary);
Formatter::updateStyle(clangFormatStyle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing. Do not fully qualify the method name.

@caretaker-claire
Copy link

@Razdeep , Reviewer has requested changes to your Pull request. I will again ask a reviewer to review it after you update this PR by pushing to PR originating branch

coder3101
coder3101 requested changes Feb 3, 2020
View reviewed changes
Copy link
Member

@coder3101 coder3101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check inline comments

@caretaker-claire
Copy link

Thank you for the changes. I have re-requested the review

@coder3101
Copy link
Member

Good. you can proceed with other files.

@Razdeep Razdeep changed the title Formatter methods arguments are passed by const refs Methods arguments are passed by const refs Feb 4, 2020
ouuan
ouuan requested changes Feb 4, 2020
View reviewed changes
Copy link
Member

@ouuan ouuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use this->? Are you familiar with classes?

@caretaker-claire
Copy link

@Razdeep , Reviewer has requested changes to your Pull request. I will again ask a reviewer to review it after you update this PR by pushing to PR originating branch

@coder3101
Copy link
Member

Why do you use this->? Are you familiar with classes?

Yes @Razdeep, I missed it too, this is only used inside members to disambiguate, there is no ambiguity in those members and hence not required to use this

@coder3101 coder3101 added work in progress The work has been started and is in progress. and removed ready_to_merge labels Feb 4, 2020
@coder3101
Copy link
Member

You will get to learn a lot of C++ Standards. Please Continue the good work ;-D

@coder3101
Copy link
Member

Maybe you could also try #150, you will get to know the source code better and also will help others to understand the code better. But if you wish to do, make sure to do it in another PR and a good per-requisite would be getting familiar with Qt Signal and Slots

@ouuan
Copy link
Member

ouuan commented Feb 4, 2020

And you should use imperative instead of descriptive in the titles of commit messages because it's brief and clear.

For example, the title of this PR should be "Pass method arguments by const refs".

You can read https://chris.beams.io/posts/git-commit/ for more guidance on writing commit messages.

@caretaker-claire
Copy link

Thank you for the changes. I have re-requested the review

@Razdeep Razdeep requested a review from ouuan February 4, 2020 20:13
@Razdeep Razdeep changed the title Methods arguments are passed by const refs Pass methods arguments by const refs Feb 4, 2020
ouuan
ouuan approved these changes Feb 5, 2020
View reviewed changes
Copy link
Member

@ouuan ouuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for your contribution!

@ouuan ouuan merged commit dbb6312 into cpeditor:master Feb 5, 2020
@Razdeep Razdeep deleted the const_ref branch February 6, 2020 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ouuan ouuan ouuan approved these changes

@coder3101 coder3101 Awaiting requested review from coder3101

Assignees
No one assigned
Labels
work in progress The work has been started and is in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Razdeep @coder3101 @ouuan