TulipCharts / tulipindicators

Technical Analysis Indicator Function Library in C
https://tulipindicators.org/
GNU Lesser General Public License v3.0
818 stars 154 forks source link

Compilation issue under VS2019 #104

Closed ludekvodicka closed 8 months ago

ludekvodicka commented 3 years ago

Hello, I just tried to upgrade to 0.9 and getting the following errors:

1>copp.c(58,87): error C2057: expected constant expression 1>copp.c(58,87): error C2466: cannot allocate an array of constant size 0 1>copp.c(58,89): warning C4034: sizeof returns 0

It's caused by this expression:

TI_REAL *roc_short = malloc(sizeof(TI_REAL[roc_short_len]));

The root of the problem is that roc_short_len :

The sizes of array variables in C must be known at compile time. If you know it only at run time you will have to malloc some memory yourself instead.

so I believe the correct way is:

TI_REAL roc_short = malloc(sizeof(TI_REAL) roc_short_len);

I update all files where this wrong expression was used and executed make a compilation. I believe everything works as expected including all tests.

Are you interested in PR with these changes?

codeplea commented 3 years ago

I guess you are in the 0.9 branch? There are still a lot of bugs in that branch. About 20 indicators were contributed, but without sufficient testing or documentation. Copp is one of them.

I would recommend sticking to master unless there is something you specifically need from 0.9. Currently, a few indicators in 0.9 even have memory errors under certain circumstances.

Thanks for the report, but there's no need to bother with the PR. The indicators in question in 0.9 will need to undergo serious refactoring before they can be accepted to master. Now if anyone is interested in doing that refactoring, that would be awesome, but it's a big project.

ludekvodicka commented 3 years ago

Thanks for the reply and info about 0.9 status. I don't need any specific 0.9 feature, I was just curious and want to check new indicators.

Is there any risk when I use mostly indicators from the master but keep with 0.9 to be able to try also the new ones? I'm still in the early development phase of my tool (cryptocurrency market analyzer) so I can live with the risk of the crashes/memory leaks.

codeplea commented 3 years ago

You should be fine in that case.

hmG3 commented 1 year ago

Hello, got the latest 0.9.1 version from Master branch and also getting similar errors as @ludekvodicka mentioned.

Error   C2057   expected constant expression            sma.c   83
Error   C2466   cannot allocate an array of constant size 0 sma.c   83
Warning C4034   sizeof returns 0                sma.c   83

I believe there should be no bugs but still have a compilation error. The line that causes an error: https://github.com/TulipCharts/tulipindicators/blob/85147c3db868dca9880f78b26de9f4fcecf95fb9/indicators/sma.c#L83

For me the fix was: *stream = malloc(sizeof(ti_stream_sma) * period);

Could you confirm if it's a problem or I just have specific local build settings?

codeplea commented 1 year ago

For me the fix was: stream = malloc(sizeof(ti_stream_sma) period);

The fix you are using I believe will create a serious memory bug.

I think you could try this:

*stream = malloc(sizeof(ti_stream_sma) + sizeof(TI_REAL) * period);

Please let me know how that works.

hmG3 commented 1 year ago

Ah, you are absolutely right.

I just missed part of the expression during copy\pasting. I have the fix exactly like that.