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

Code Cleanup #66

Merged
merged 6 commits into from
Nov 11, 2017
Merged

Code Cleanup #66

merged 6 commits into from
Nov 11, 2017

Conversation

fuzzard
Copy link
Contributor

@fuzzard fuzzard commented Sep 22, 2017

Just sets all files to use spaces instead of tab. The majority already uses this, so figure it all might as well match. I guess im a bit ocd, so feel free to reject. There is absolutelly NO functional changes in the whitespace fixup.

The execution change in Update() is to create the lock earlier, and exit if tuner discovery failure earlier. should have very little affect, but figure it might be a little nicer.

@fuzzard fuzzard changed the title Minors/Whitespace fixup Code Cleanup Nov 10, 2017
@fuzzard
Copy link
Contributor Author

fuzzard commented Nov 10, 2017

decided to push some more changes to this PR. Based around modernising existing code.
Changes for loops to auto ranging, which reduces the temp variable usage. Cleanup some unused variables. Changed memset for 0 initialization to aggregate initialization (http://en.cppreference.com/w/cpp/language/aggregate_initialization)

@fuzzard
Copy link
Contributor Author

fuzzard commented Nov 10, 2017

ping @ksooo
I know your a busy man, but mind having a look when you have a spare moment.

Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

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

Just had a quick look, lots of stuff changed, not enough time to do a real review.

Please update the changelog and do an addon version bump. Then we can merge stuff (and will see what happens).

@ksooo
Copy link
Member

ksooo commented Nov 11, 2017

Could you please remove the merge commit and do a rebase instead?

@fuzzard
Copy link
Contributor Author

fuzzard commented Nov 11, 2017

version bump and changelog added.

There is a lot of churn, mostly to do with the whitespaces commit. If its preferred i can just drop that. It was just to make all files uniform, and really isnt that important.

@ksooo
Copy link
Member

ksooo commented Nov 11, 2017

For the changelog, not every single code cleanup change needs to be documented in detail. A one liner "Code cleanup (no functional changes)" would imo be enough.

@ksooo
Copy link
Member

ksooo commented Nov 11, 2017

Leave the whitespace changes in, makes sense. Just remove the merge commit...

changed to set lock sooner, and also fail out sooner if
hdhomerun_discover_find_devices_custom_v2 returns -1.
Allows to remove a bunch of temp iterator variables and use auto range loops where
appropriate. Cleaned up some unused variables.
as these objects were created, and we are failing, we should really clean them up first
@fuzzard
Copy link
Contributor Author

fuzzard commented Nov 11, 2017

Think thats all better.

I do appreciated the help, thanks.

@ksooo
Copy link
Member

ksooo commented Nov 11, 2017

Thanks for your contribution.

@ksooo ksooo merged commit 82223c0 into kodi-pvr:master Nov 11, 2017
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.

2 participants