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

Implement stumpless_element_to_string function #180

Conversation

northernSage
Copy link
Contributor

@northernSage northernSage commented May 28, 2021

This addresses #146

  • add declaration to element.h
  • implement stumpless_element_to_string in element.c
  • add tests

@northernSage
Copy link
Contributor Author

northernSage commented May 28, 2021

I have created the draft to open what I have done so far for review (early feedback makes for a less painful process :bowtie: ) In case anyone has suggestions on how to improve the PR feel free to let me know.

@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #180 (44624e9) into latest (cbd8dd5) will increase coverage by 0.01%.
The diff coverage is 97.61%.

❗ Current head 44624e9 differs from pull request most recent head e95b0b2. Consider uploading reports for the commit e95b0b2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           latest     #180      +/-   ##
==========================================
+ Coverage   95.90%   95.91%   +0.01%     
==========================================
  Files          31       31              
  Lines        2589     2620      +31     
==========================================
+ Hits         2483     2513      +30     
- Misses        106      107       +1     
Impacted Files Coverage Δ
src/element.c 98.28% <97.61%> (-0.12%) ⬇️
src/entry.c 97.97% <0.00%> (-0.07%) ⬇️
src/validate.c 100.00% <0.00%> (ø)
src/strbuilder.c 87.73% <0.00%> (+1.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbd8dd5...e95b0b2. Read the comment docs.

@goatshriek
Copy link
Owner

goatshriek commented May 28, 2021

Looks like you're on a good path! Right now the only comment I have is to be sure that you free the result of param_to_string after you memcpy it into the new buffer, otherwise it will become a memory leak.

@northernSage
Copy link
Contributor Author

Thanks for the heads up, will get it sorted and finished the PR in the next few days 👍🏻

@northernSage
Copy link
Contributor Author

This should be ready for review, feel free to suggest any other improvements or corrections @goatshriek

@goatshriek
Copy link
Owner

Looks like you're pretty much there, awesome work on putting this together! My apologies for the delay in running the CI actions, I forgot that they must be manually approved now. Just a few things left to clean up:

The most difficult issue is that the Microsoft compiler doesn't appear to support the non-compile-time constant size in the array declaration on line 525 of src/element.c, which you probably already noticed from the failed actions. You could allocate that array from the heap instead, append the strings as you iterate through the params one by one instead of storing all of the strings first, or take a different approach entirely, it's up to you.

You'll need to add an entry to the src/windows/stumpless.def file so that the function is exported in the DLL, otherwise the Windows tests will continue to fail even after the compilation issue is resolved. Just tack the new function name on to the end of the file with the next available ordinal number and you should be all set.

The branch covering elements without any params is currently uncovered - adding another test almost identical to the one starting on line 712 of test/function/element.cpp but with the basic_element fixture element instead of the element_with_params one should cover this.

You can ignore the sonarcloud failure - it won't run for "external" pull requests at the moment.

@northernSage
Copy link
Contributor Author

northernSage commented Jun 5, 2021

Thanks for pointing out the issues in such detail, it helped a lot! I believe this is ready for another go :bowtie:

@northernSage northernSage marked this pull request as ready for review June 5, 2021 01:45
@goatshriek
Copy link
Owner

Looks great! the mac failures look like a false alarm - I will give this one more check in a few hours and merge it in!

@goatshriek goatshriek merged commit 89dd69c into goatshriek:latest Jun 6, 2021
@northernSage northernSage deleted the implement-element-to-string-function branch June 6, 2021 02:23
@northernSage
Copy link
Contributor Author

northernSage commented Jun 6, 2021

From #146:

You may consider refactoring some common portions of stumpless_param_to_string into separate private functions that can be used in both places for efficiency...

Should I open a follow up issue for this? I would be willing to work on it after #177 @goatshriek

@goatshriek
Copy link
Owner

If you're interested, absolutely! There is a detailed walkthrough of making/measuring performance optimizations in docs/benchmark.md that walks through the general flow used to make/test speed improvements.

There's no need to feel obligated though; I can always write another good first issue for the improvements as well. Totally up to you!

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.

None yet

2 participants