Skip to content

Conversation

AbdelrahmanKhaledd
Copy link
Collaborator

No description provided.

}
res[len] = '\0';
return res;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This function I think we can implement in LPython, so we should, and remove it from ASR.

We should only put things into ASR that cannot be implemented in LPython.

Copy link
Collaborator Author

@AbdelrahmanKhaledd AbdelrahmanKhaledd Aug 15, 2022

Choose a reason for hiding this comment

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

@certik i've implemented like _lpython_floordiv is that the correct way?

@AbdelrahmanKhaledd AbdelrahmanKhaledd force-pushed the str_cap branch 3 times, most recently from bd394d5 to c485352 Compare August 14, 2022 23:56
@AbdelrahmanKhaledd AbdelrahmanKhaledd force-pushed the str_cap branch 4 times, most recently from c0d3af1 to 0eb7467 Compare August 15, 2022 23:04
@certik
Copy link
Contributor

certik commented Aug 17, 2022

@Abdelrahman-Kh-Fouad is this ready for review?

@AbdelrahmanKhaledd AbdelrahmanKhaledd changed the title strings capitalize strings: capialize, lower Aug 17, 2022
@AbdelrahmanKhaledd
Copy link
Collaborator Author

@Abdelrahman-Kh-Fouad is this ready for review?

yes it's

ASR::ttype_t *type = ASRUtils::TYPE(ASR::make_Character_t(al, loc,
1, 1, nullptr, nullptr, 0));
ASR::ttype_t *res_type = ASRUtils::TYPE(ASR::make_StringConstant_t(al, loc, s2c(al, ""), type));
return ASR::down_cast<ASR::expr_t>(ASR::make_StringConstant_t(al, loc, s2c(al, val), res_type));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the code here implement the capitalization at compile time? It seems it just repackages the first argument as StringConstant, but does not capitalize.

Copy link
Collaborator Author

@AbdelrahmanKhaledd AbdelrahmanKhaledd Aug 17, 2022

Choose a reason for hiding this comment

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

yeah, i think i forgot, i updated it from seconds, shall i reabse the branch now or wating for review?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, just keep adding commits until it is completely reviewed and ready. Only as the very last thing before merge you can rebase / polish the history.

@certik
Copy link
Contributor

certik commented Aug 17, 2022

I think this is generally good, thanks for implementing it!

}
AST::ConstantStr_t *n = AST::down_cast<AST::ConstantStr_t>(at->m_value);
std::string res = n->m_value;
res[0] = toupper(res[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it is an empty string?

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 added it.

s: str
s = "AaaaAABBbbbbBB!@12223BN"
assert s.lower() == "aaaaaabbbbbbbb!@12223bn"
assert "DDd12Vv" .lower() == "ddd12vv"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused here --- I would expect this line to fail until you actually implemented the compile time version in 6c7edc8, but it seemed it was passing --- is there a bug somewhere? Is this not exercising the compile time version?

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is fine.

@certik certik merged commit 318a6fe into lcompilers:main Aug 20, 2022
@AbdelrahmanKhaledd AbdelrahmanKhaledd changed the title strings: capialize, lower Strings: capialize(),lower() Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants