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

Eliminate code duplication and improve documentation: #3924

Closed
wants to merge 1 commit into from
Closed

Eliminate code duplication and improve documentation: #3924

wants to merge 1 commit into from

Conversation

nbougalis
Copy link
Contributor

The legacy functions cdirFirst and dirFirst were mostly identical; the differences were only type-related. The same situation existed with cdirNext and dirNext.

This commit removes the duplicated code by introducing new template functions that abstract away the differences that are present between each pair of functions.

This commit also improves the naming of function arguments, helping to elucidate their purpose & use and to make the code self-documenting.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@nbougalis nbougalis added the Tech Debt Non-urgent improvements label Sep 3, 2021
The legacy functions `cdirFirst` and `dirFirst` were mostly
identical; the differences were only type-related. The same
situation existed with `cdirNext` and `dirNext`.

This commit removes the duplicated code by introducing new
template functions that abstract away the differences that
are present between each pair of functions.

This commit also improves the naming of function arguments,
helping to elucidate their purpose & use and to make the
code self-documenting.
@scottschurr scottschurr self-assigned this Sep 3, 2021
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍 Looks like a good change to me. Should be all set to go once clang-format is happy.

@@ -45,8 +45,7 @@ class Book_test : public beast::unit_test::suite
sleOfferDir->key(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clang-format wants to reformat these arguments into a single line. That change will have to happen before we can merge this.

@manojsdoshi manojsdoshi mentioned this pull request Sep 9, 2021
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

This looks good, but I'd like to discuss the enable_if before giving a thumbs up.


@deprecated These are legacy function that are considered deprecated
and will soon be replaced with an iterator-based model
that is easier to use. You should not use them in new code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we can deprecate something until the replacement it available.

class N,
class = std::enable_if_t<
std::is_same_v<std::remove_cv_t<N>, SLE> &&
std::is_base_of_v<ReadView, V>>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider removing the enable_if. Rational: we aren't using enable_if to resolve an overload, we're using it as a type-check and as documentation. I'd probably leave it off entirely and add a comment. This is a detail function only called from this file (it's not on a public interface where clients can call it with weird parameter types and wonder what's wrong). If the wrong types are passed in this file, we'll still get a compile error and quickly figure out why.

If you don't like the comment, as an alternative, consider adding a static_assert. I'd prefer we didn't increase our use of enable_if to cases like this. When we see enable_if I want to think "this is part of an overload set".

@nbougalis nbougalis mentioned this pull request Sep 9, 2021
@nbougalis nbougalis deleted the legacyview branch October 16, 2023 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech Debt Non-urgent improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants