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

Adler crc hash #444

Merged
merged 9 commits into from
Feb 3, 2024
Merged

Adler crc hash #444

merged 9 commits into from
Feb 3, 2024

Conversation

RhoSigma-QB64
Copy link
Member

This 1st PR will implement the new functions _ADLER32, _CRC32 and _MD5$, after it is successfully merged I'll start a 2nd PR which then exchanges the use of the recently added GetCRC32&() BASIC FUNCTION (utilities/strings.bas) with _CRC32 or probably even _MD5$ and remove that BASIC FUNCTION again.

- disabling some unneeded features saves ~52KiB object size (55%)
- exposing the implementations in miniz for public use
- exposing the implementations in freetype for public use
@RhoSigma-QB64 RhoSigma-QB64 self-assigned this Feb 2, 2024
@RhoSigma-QB64 RhoSigma-QB64 added the enhancement New feature or request label Feb 2, 2024
Copy link
Contributor

@mkilgore mkilgore left a comment

Choose a reason for hiding this comment

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

Can you add some tests showing the new functionality working?

id.callname = "func__adler32"
id.args = 1
id.arg = MKL$(STRINGTYPE - ISPOINTER)
id.ret = LONGTYPE - ISPOINTER
Copy link
Contributor

Choose a reason for hiding this comment

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

The return type for these functions should be ULONGTYPE.

@@ -7,6 +7,16 @@
#include "../../libqb.h"
#include "miniz.h"

uint32_t func__adler32(qbs *text) {
if (!text->len) return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it return 1 when the text is blank?

@@ -9,5 +9,7 @@

struct qbs;

uint32_t func__adler32(qbs *text);
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than put them in font.h and compression.h I think it would make more sense to add a new hashing.h and put them in there.

@a740g
Copy link
Contributor

a740g commented Feb 3, 2024

Can you add some tests showing the new functionality working?

I agree. A simple test with a minimum of 6 function calls in total should be conducted. For each of the 3 functions, we need two tests: one with an empty string and another with some pre-defined string.

Also like @mkilgore pointed, adler32 for an empty string should return a 1.

- return type is uint32_t in C/C++ and ULONGTYPE in QB64
- adler32 must return one (1) on an empty input
- hopefully
- ok, that was my fault?, no it wasn't,
- console is ignoring trailing space when copying text from it
/// @param text The message to build the MD5 hash of
/// @return The generated MD5 hash as hexadecimal string
qbs *func__md5(qbs *text) {
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it really matters, but we can get rid of the extra braces.

@RhoSigma-QB64 RhoSigma-QB64 merged commit 781250b into main Feb 3, 2024
4 checks passed
@RhoSigma-QB64 RhoSigma-QB64 deleted the adler-crc-hash branch February 3, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants