zpm-project / zpm-zsh

zsh plugin manager in ansi C.
GNU General Public License v3.0
5 stars 3 forks source link

Valgrind #27

Closed fennecdjay closed 7 years ago

fennecdjay commented 7 years ago

Also a few improvements:

fennecdjay commented 7 years ago

feea535 fails on OSX valgrind tests. Showing the log and making valgrind more verbose on failure would ease debugging.

desyncr commented 7 years ago

Well this is exactly what we're looking for! To have valgrind to cover us in regard of memory leaks and hidden bugs. So it's all fine!

Now, do you want to try to fix the memory issues in this PR or should I merge this already (haven't done reviewing it yet, still) and we'll catch the memory issues later on?

fennecdjay commented 7 years ago

we'll catch the memory issues later on? I'd prefer that, but we should not merge before we see the valgrind log on errors. I'll work on this ASAP.

fennecdjay commented 7 years ago

Sorry for the delay. I think now (with allowed_failures and loggin on errors), it'll be fine.

desyncr commented 7 years ago

I'll try this later tonight. Looking fine and it's a great addition. My only nitpick is having to use $ZPM in cram tests. Can aliases be used instead?

Regards for v0.1.0 @fennecdjay https://github.com/zpm-project/zpm-zsh/releases/tag/v0.1.0

fennecdjay commented 7 years ago

Can aliases be used instead?

I'd like that, but never managed to do so 😕 Any hint appreciated here 😄

fennecdjay commented 7 years ago

Now all leaks come from url.h. Maybe we should fix it (along with for loop declarations) in a fork and make a pull request. What do you think? (I'm not even sure where to discuss this 😖 )


EDIT: you can have a look a jwerle/url.h#3.

I tend to think we could ship a refined zpm-project version of this.

fennecdjay commented 7 years ago

Can aliases be used instead?

I've been rechecking cram's doc and issue tracker, and did not find a clue on how to do that. From what I understand, cram launches a new shell on each test file. All my previous trials failed, so unless you got a way to achive this that you can show me, I think we'll have to stick to the $ZPM thing.

desyncr commented 7 years ago

@fennecdjay We can contribute to url.h project, either by raising an issue or creating a PR (it's always better to start with an issue to check if there is an interest from the other side).

In regard of aliases I couldn't do it right away. I'm OK with $ZPM if there is not another option.

fennecdjay commented 7 years ago

I created an issue, and a PR would be welcome :tada:. see jwerle/url.h#4

desyncr commented 7 years ago

@fennecdjay Yay! Awesome!

fennecdjay commented 7 years ago

I'll fix this soon. Should we use something like [astyle](), or whichever you prefer, along with a config file to get rid of those annoyances?

fennecdjay commented 7 years ago

I think there nothing else to add here. we could merge and move to next steps.

desyncr commented 7 years ago

Awesome! 👍 Great addition!