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

[bugs] fixes for lgtm and improve docs #553

Merged
merged 6 commits into from
Jul 2, 2020
Merged

[bugs] fixes for lgtm and improve docs #553

merged 6 commits into from
Jul 2, 2020

Conversation

kvedala
Copy link
Collaborator

@kvedala kvedala commented Jul 2, 2020 •
edited
Loading

Description of Change

  • project Euler problem 22 lgtm error fix
  • fix inconsistent use of double and long double in durand_kerner_roots.c
  • add docs & fix lgtm alerts in bead_sort.c

References

Checklist

  • Added description of change
  • Added file name matches File name guidelines
  • Added tests and example, test must pass
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • Search previous suggestions before making a new one, as yours may be a duplicate.
  • I acknowledge that all my contributions will be made under the project's license.

Notes:

@kvedala kvedala changed the title fixes/lgtm [bugs] fixes for lgtm and improve docs Jul 2, 2020
tjgurwara99
tjgurwara99 reviewed Jul 2, 2020
View reviewed changes
Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

This works too😂 I didn't see it this way.

There is another file numerical_methods/durand_kerner_roots.c which has a similar issue on line 50 where the type of coeff is double instead of long double.

@kvedala
Copy link
Collaborator Author

kvedala commented Jul 2, 2020

This works too😂 I didn't see it this way.

There is another file numerical_methods/durand_kerner_roots.c which has a similar issue on line 50 where the type of coeff is double instead of long double.

rule of thumb: avoid typecasting and find and address the root-problem :)

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request fixes 1 alert when merging f21f18e into 0f48961 - view on LGTM.com

fixed alerts:

  • 1 for Multiplication result converted to larger type

@tjgurwara99
Copy link
Member

I can approve this if you want to create a separate PR for durand_kerner_roots.c @kvedala

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request introduces 2 alerts and fixes 2 when merging cb99925 into 0f48961 - view on LGTM.com

new alerts:

  • 2 for Wrong type of arguments to formatting function

fixed alerts:

  • 2 for Multiplication result converted to larger type

tjgurwara99
tjgurwara99 reviewed Jul 2, 2020
View reviewed changes
numerical_methods/durand_kerner_roots.c
coeffs = (double *)malloc(
degree * sizeof(double)); /* store all input coefficients */
coeffs = (long double *)malloc(
degree * sizeof(long double)); /* store all input coefficients */
Copy link
Member

@tjgurwara99 tjgurwara99 Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

printf might complain now on line 150 and 152

Copy link
Member

Choose a reason for hiding this comment

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

oh LGTM issued the alert

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, converted to draft. dont review until marked as ready for review.

@kvedala kvedala marked this pull request as draft July 2, 2020 00:50
@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request fixes 2 alerts when merging 2150c35 into 0f48961 - view on LGTM.com

fixed alerts:

  • 2 for Multiplication result converted to larger type

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request fixes 3 alerts when merging ec57c8f into 0f48961 - view on LGTM.com

fixed alerts:

  • 3 for Multiplication result converted to larger type

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request fixes 7 alerts when merging ccd3f66 into 0f48961 - view on LGTM.com

fixed alerts:

  • 3 for Multiplication result converted to larger type
  • 2 for Local variable hides global variable
  • 2 for Short global name

@kvedala kvedala marked this pull request as ready for review July 2, 2020 01:57
tjgurwara99
tjgurwara99 approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Everything looks good

@kvedala kvedala merged commit 9a58248 into TheAlgorithms:master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjgurwara99 tjgurwara99 tjgurwara99 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kvedala @tjgurwara99