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

Refactor .extend() and .remove() to use FExpr #3393

Merged
merged 10 commits into from Dec 15, 2022
Merged

Conversation

samukweku
Copy link
Collaborator

@samukweku samukweku commented Dec 2, 2022

WIP for #2562

@samukweku samukweku added the improve Improvement of an existing functionality label Dec 2, 2022
@samukweku samukweku self-assigned this Dec 2, 2022
@samukweku samukweku changed the title FExpr extend FExpr extend and remove Dec 2, 2022
src/core/expr/fexpr.cc Show resolved Hide resolved
private:
ptrExpr arg_;
ptrExpr values_;
bool extend_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a class attribute, we should use a template parameter bool EXTEND like we just did for SUM. This way compiler will create two classes: one for EXTEND == true and another for EXTEND == false, streamlining all the if (EXTEND) ... conditions within those classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oleksiyskononenko could you kindly have a look at my latest commit. where am I getting it wrong with my knowledge of templates

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, the way this template should be implemented is exactly the same as for many other classes we have recently created. Could you please explain me what is the issue you are facing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the error I am getting:

src/core/expr/fexpr_extend_remove.cc:28:1: error: invalid use of template-namedt::expr::FExpr_Extend_Removewithout an argument list
   28 | FExpr_Extend_Remove::FExpr_Extend_Remove(ptrExpr&& arg, ptrExpr&& other) :
      | ^~~~~~~~~~~~~~~~~~~

my google fu hasnt fetched me anything helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i basically add the template<bool EXTEND> to fexpr_extend_remove.h and fexpr_extend_remove.cc

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you keep template declarations in the .h file and all the definitions in .cc, you need to explicitly specify for which EXTEND values, i.e. true and false, you're creating implementations outside of the header file. Also, definitions of the class methods should all include the template parameter. Take a look at the changes I've just pushed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you @oleksiyskononenko ... It was hard for me to decipher this solution based on the error message. Thanks agaon

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, error messages for templates are not really helpful. The standard way to implement it is to put everything in the header file. Once you split the declarations/definitions, some issues may arise. If you need more info on this, google for “C++ template instantiation”.

src/core/expr/fexpr_extend_remove.cc Outdated Show resolved Hide resolved
src/core/expr/fexpr_extend_remove.h Outdated Show resolved Hide resolved
@oleksiyskononenko oleksiyskononenko added this to the Release 1.1.0 milestone Dec 14, 2022
@oleksiyskononenko oleksiyskononenko added the refactor Internal code changes, clean-ups or reorganizations that are not externally visible label Dec 14, 2022
@oleksiyskononenko oleksiyskononenko changed the title FExpr extend and remove Refactor .extend() and .remove() to use FExpr Dec 14, 2022
@oleksiyskononenko
Copy link
Contributor

@samukweku I pushed some changes to this PR. See if they look good to you.

@samukweku
Copy link
Collaborator Author

Thanks for the review and update @oleksiyskononenko. Looks great

@oleksiyskononenko oleksiyskononenko merged commit 9522f08 into main Dec 15, 2022
@oleksiyskononenko oleksiyskononenko deleted the samukweku/extend branch December 15, 2022 19:34
samukweku added a commit that referenced this pull request Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve Improvement of an existing functionality refactor Internal code changes, clean-ups or reorganizations that are not externally visible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants