Skip to content

feat: add boyer moore algorithm implementation #2441

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

Merged
merged 34 commits into from
Jun 16, 2023

Conversation

stoychoX
Copy link
Contributor

Description of Change

Checklist

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

Notes: This implementation is mine, but the algorithm itself is similar to one I saw on geeksforgeeks (as I learned about the algorithm from there). There are other ways to implement the good suffix table, but I truly find this one the best.

@Panquesito7 Panquesito7 added enhancement New feature or request automated tests are failing Do not merge until tests pass labels Mar 15, 2023
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Almost there! 😄

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Please fix the clang-tidy warnings.
Let us know if you need any help with that. 🙂

@Panquesito7 Panquesito7 added the requested changes changes have been requested label Apr 10, 2023
@Panquesito7 Panquesito7 removed the automated tests are failing Do not merge until tests pass label Apr 14, 2023
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Tested on Gitpod and the tests seem to be failing. Please fix them.

image

@stoychoX
Copy link
Contributor Author

Hmm yes, there was error with size_t overflowing. I think I tested it many times and it did its job I don't know what got wrong this time, but I fixed the type problem.

image

@stoychoX
Copy link
Contributor Author

Found the problem! As I was fixing clang-tidy warnings I changed a type which should not be changed.

Now tests are fine and I dont see any clang-tidy warnings.

@Panquesito7 Panquesito7 requested review from realstealthninja and tjgurwara99 and removed request for realstealthninja May 26, 2023 21:49
Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contributions! Just a little bit more ❤️

@realstealthninja
Copy link
Collaborator

realstealthninja commented May 30, 2023

also @Panquesito7 there seems to be both @ return and @ returns in the contribution guidelines and both are used interchangeably?
https://wx-dev.narkive.com/w3HIen0a/doxygen-docs-return-or-returns
should we standardize and use either return or returns or keep it as is?

Co-authored-by: realstealthninja <[email protected]>
stoychoX and others added 7 commits May 30, 2023 18:53
Co-authored-by: realstealthninja <[email protected]>
Co-authored-by: realstealthninja <[email protected]>
Co-authored-by: realstealthninja <[email protected]>
Co-authored-by: realstealthninja <[email protected]>
Co-authored-by: realstealthninja <[email protected]>
Co-authored-by: realstealthninja <[email protected]>
Co-authored-by: realstealthninja <[email protected]>
Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

Just a little bit more!!! 🙂

@Panquesito7
Copy link
Member

also @Panquesito7 there seems to be both @ return and @ returns in the contribution guidelines and both are used interchangeably? https://wx-dev.narkive.com/w3HIen0a/doxygen-docs-return-or-returns should we standardize and use either return or returns or keep it as is?

Good question. I think we might want to standardize the use of return, rather than returns.
I'll create a discussion for this, so we can see what others think about this. 🙂

tjgurwara99
tjgurwara99 previously approved these changes Jun 5, 2023
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.

LGTM

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Amazing work! Thank you. 🚀

Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

Great work! ❤️

@Panquesito7 Panquesito7 merged commit d7a9869 into TheAlgorithms:master Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requested changes changes have been requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants