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

EIP-969: style corrections + syntax HLing + make C-code blob a drop-in snippet. #979

Merged
merged 6 commits into from Apr 16, 2018

Conversation

veox
Copy link
Contributor

@veox veox commented Apr 6, 2018

Mostly removes trailing whitespace and adds hyphens.


EDIT: Now also edits the C snippet to be an actual program that can be compiled.

Mostly removes trailing whitespace and adds hyphens.
@veox
Copy link
Contributor Author

veox commented Apr 6, 2018

The C-code blob fails to compile for me.

Other than type names being slightly different in my gcc (7.3.1), this line:

 uint32_t result = (uint_32)(candidate * fnv_candidate);

Has (uint_32), which is not a recognised type.

Also, the loop shown in current master HEAD 9fba86e5 is endless. X_X


This compiles (and has the infinite-loop fixed):

// http://eips.ethereum.org/EIPS/eip-969

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

int main() {
    u_int32_t candidate = 0;
    u_int32_t dups = 0;
    u_int32_t fnv_candidate = 0x10001a7; // MODIFY!
    u_int8_t *counts = malloc(0xFFFFFFFF);

    memset(counts, '\0', 0xFFFFFFFF);

    for (candidate = 0; candidate < 0xFFFFFFFF; candidate++) {
        u_int32_t result = (u_int32_t)(candidate * fnv_candidate);
        u_int8_t oldCount = counts[result];

        counts[result] = counts[result]+1;
        if (oldCount != 0) {
            dups++;
        }

        //// progress display: remove comment to speed down
        //if ((candidate & 0xFFFFF) == 0xFFFFF) printf("0x%08x\n", candidate);
    }
    printf("\nFNV candidate 0x%08x : %i dups\n", fnv_candidate, dups);

    return 0;
}

It gives a result of "0 dups". Expected?.. No?..

@veox
Copy link
Contributor Author

veox commented Apr 6, 2018

To clarify: I'm trying to run the code provided, since

It can be empirically confirmed that no more than 1 duplicates occur in the 32 bit word space with these constants.

suggests empirical confirmation, yet contains a typo: either "no more than 1??? duplicates occur", or "no more than 1 duplicate occurs".


EDIT: My understanding of the code above suggests the latter.

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

@veox
Copy link
Contributor Author

veox commented Apr 6, 2018

Should I add the changes outlined above to this PR, or open a new one?..

@AirSquirrels
Copy link
Contributor

Yes sorry I retyped the EIP originally as pseudo code for clarity (my original was a bit messy, iterating through a larger number of constants) in the EIP, but ended up being close to complete C code. The mechanism is the same. Note the constant is also written for only one of the FNV constant, each should be evaluated

veox added 2 commits April 9, 2018 17:22
See

ethereum#979 (comment)

for what was wrong with the code. Gist:

* endless loop;
* typo in type-cast.
@veox veox changed the title EIP-969: style/syntax corrections + syntax HLing. EIP-969: style corrections, + syntax HLing + make C-code blob a drop-in snippet. Apr 9, 2018
@veox veox changed the title EIP-969: style corrections, + syntax HLing + make C-code blob a drop-in snippet. EIP-969: style corrections + syntax HLing + make C-code blob a drop-in snippet. Apr 9, 2018
@veox
Copy link
Contributor Author

veox commented Apr 9, 2018

OK.

I've incorporated the changes you've made in the interim (github's merge-master strategy happily wiped them instead of warning of incompatibility); and put the C program in as a copy-paste snippet.

It doesn't loop through the proposed constants, in order to keep the snippet short. It'd maybe make sense to read those from stdin.

@veox
Copy link
Contributor Author

veox commented Apr 16, 2018

@AirSquirrels As I understand, as the EIP's champion, you can merge this PR. No one else is going to assume duty to do so.

If you find the changes acceptable, please do so. If not, and there's some unaddressed concern you have, please voice it. :)

@AirSquirrels
Copy link
Contributor

@veox I approve of these changes, however I am not well versed in the processes here for giving my approval to the eip-automerger. I do not have permissions to merge into the main repo.

@Arachnid
Copy link
Contributor

@AirSquirrels Click on "Files Changed", then "Review Changes", click the "Approve" radio box, and submit.

@AirSquirrels
Copy link
Contributor

@Arachnid Thanks, I see that now.

@eip-automerger eip-automerger merged commit 822eec1 into ethereum:master Apr 16, 2018
@veox veox deleted the patch-2 branch April 16, 2018 16:16
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

4 participants