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

Confusing argument names dynRes and inputDyn #377

Closed
hannahbast opened this issue Apr 6, 2021 · 1 comment
Closed

Confusing argument names dynRes and inputDyn #377

hannahbast opened this issue Apr 6, 2021 · 1 comment
Assignees
Labels
cleanup Non functional change for better maintainability or portability

Comments

@hannahbast
Copy link
Member

In many Operations, we have the following beginning and end of functions:

void Bind::computeRenameBind(IdTable* dynRes, const IdTable& inputDyn,
                             size_t column) {
  const auto input = inputDyn.asStaticView<IN_WIDTH>();
  auto res = dynRes->moveToStatic<OUT_WIDTH>();
  ...
  *dynRes = res.moveToDynamic();
}

This is confusing for a number of reasons:

  1. Why res and not result
  2. Why is dyn used as prefix and suffix
  3. Why is dynRes not returned, but a * argument (there might be a good reason for this one)
@hannahbast hannahbast added the cleanup Non functional change for better maintainability or portability label Apr 6, 2021
@hannahbast
Copy link
Member Author

The respective code has been refactored a while ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non functional change for better maintainability or portability
Projects
None yet
Development

No branches or pull requests

2 participants