search

openitu / STL

The ITU-T Software Tool Library (G.191)
https://www.itu.int/rec/T-REC-G.191/en
Other
82 stars 26 forks source link

modify basop files for EVS codec floating-point source usage #157

Closed signalogic closed 1 year ago

signalogic commented 1 year ago

basop32_evs.c and basop32_evs.h are modified basop32 files for use with ETSI 3GPP EVS floating-point source code files that include basop calls. The modifications move some functions to basop32_evs.h as static inline, disable terminations not suitable for Linux libs such as exit() and provide alternate error handling and message logging, and remove or alter use of global variables Overflow and Carry. For example, sub_ovf() is identical to sub() but includes a stack based Overflow param. Definitions such as USE_BASOPS_INLINE, EXCLUDE_BASOPS_NOT_USED_EVS, USE_BASOPS_OVERFLOW_GLOBAL_VAR, USE_BASOPS_CARRY_GLOBAL_VAR, and USE_BASOPS_EXIT allow fine-grain control of modification behavior. Detailed comments are included

dennisguse commented 1 year ago

Let's start simple. First of all, what is the problem you are solving? Why should it be included in ITU-T's STL?

Observation:

signalogic commented 1 year ago

There are code cleanups happening within these commits. Why?

I assume you mean formatting, indentation, comments, etc ? If so we applied our format rules to be consistent with other codec source files. If you need a version with only functional mods we can provide.

There is no test added for this code, right?

Right, at least not a minimal functional test. The code is tested extensively as part of our EVS codec, and several reference / test programs that use the codec.

simaocampos commented 1 year ago

I suggest keeping the original indentation to facilitate review and minimize surprises.

Indeed the lack of test files has been a systemic problem for this module since the begining when it was first added. But this is mitigated as it is used as part of the code for reference implementation of codecs, so problems could be caught in that context.

Simao

Forgive terseness, composed on a mobile phone.

-------- Original message -------- From: "Signalogic, Inc." @.> Date: 3/29/23 21:58 (GMT+01:00) To: openitu/STL @.> Cc: Subscribed @.***> Subject: Re: [openitu/STL] modify basop files for EVS codec floating-point source usage (PR #157)

There are code cleanups happening within these commits. Why?

I assume you mean formatting, indentation, comments, etc ? If so we applied our format rules to be consistent with other codec source files. If you need a version with only functional mods we can provide.

There is no test added for this code, right?

Right, at least not a minimal functional test. The code is tested extensively as part of our EVS codec, and several reference / test programs that use the codec.

— Reply to this email directly, view it on GitHubhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenitu%2FSTL%2Fpull%2F157%23issuecomment-1489221634&data=05%7C01%7Csimao.campos%40itu.int%7C8ebdcc3daf1c4108213d08db308ff3e7%7C23e464d704e64b87913c24bd89219fd3%7C0%7C0%7C638157167042495650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ddTvkoCNTGI0azud9aI%2FZ66HJ3atdCN0hjy76Qiyx3Q%3D&reserved=0, or unsubscribehttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEC4YAWP525GUQC2PCTWCGLW6SH55ANCNFSM6AAAAAAWELLKMM&data=05%7C01%7Csimao.campos%40itu.int%7C8ebdcc3daf1c4108213d08db308ff3e7%7C23e464d704e64b87913c24bd89219fd3%7C0%7C0%7C638157167042495650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lP6RagHlHCcaRjuK6FmoTls54YU%2B4T3cyLyXGt1qpJg%3D&reserved=0. You are receiving this because you are subscribed to this thread.Message ID: @.***>

signalogic commented 1 year ago

Hi Simao, ok we'll resubmit. Should we remove the current pull requests ?

Can we keep the comments ? They are substantial but important in explaining the mods.

dennisguse commented 1 year ago

@signalogic you can just overwrite the PR by git push -f

simaocampos commented 1 year ago

Thanks for the reply. I think the comments are useful, at least for now. Simão

From: Signalogic, Inc. @.> Sent: Thursday, 30 March, 2023 03:42 To: openitu/STL @.> Cc: Campos, Simao @.>; Comment @.> Subject: Re: [openitu/STL] modify basop files for EVS codec floating-point source usage (PR #157)

Hi Simao, ok we'll resubmit. Should we remove the current pull requests ?

Can we keep the comments ? They are substantial but important in explaining the mods.

- Reply to this email directly, view it on GitHubhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenitu%2FSTL%2Fpull%2F157%23issuecomment-1489564783&data=05%7C01%7Csimao.campos%40itu.int%7C5ca049cf913241943c1108db30c00169%7C23e464d704e64b87913c24bd89219fd3%7C0%7C0%7C638157373433706369%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2Bmnq%2BBWc0unxKi0mxNyux%2Bv8zX%2B7q9jz9xqQCZiqrOQ%3D&reserved=0, or unsubscribehttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEC4YAQWYBVHE2YL7GXPYELW6TQHZANCNFSM6AAAAAAWELLKMM&data=05%7C01%7Csimao.campos%40itu.int%7C5ca049cf913241943c1108db30c00169%7C23e464d704e64b87913c24bd89219fd3%7C0%7C0%7C638157373433706369%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=LfXp8fDTAqZQs%2BT7HMpfGcJRHpOsjfTVYu2NwnzW2W4%3D&reserved=0. You are receiving this because you commented.Message ID: @.**@.>>

signalogic commented 1 year ago

updated pull request submitted, with no cleanup on format & indentation