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

make cases for lowstar_endianness consistent #252

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

franziskuskiefer
Copy link
Member

I don't mind which version to use but the warnings about this are annoying, let's decide on a casing.

@msprotz
Copy link
Contributor

msprotz commented Apr 14, 2022

PascalCase = kremlin-generated file
snake_case = hand-written

there are two such files: the snake_case one is in include/kremlin, the PascalCase one is in dist/generic/*

The correct fix is probably to rename the snake_case one to something more meaningful, such as lowstar_endianness_builtins.h to indicate that it contains hand-written prototypes that "fill out" the endianness functions on a per-platform basis.

The auto-generated one contains function prototypes to be implemented by hand and allows making sure that the implementation of e.g. load128_le (in kremlib/c/fstar_uint128_gcc64.h) is correct with regards to what is claimed in the F* file (as extracted in kremlib/dist/generic/LowStar_Endianness.h).

Hope this makes sense!

Copy link
Contributor

@msprotz msprotz left a comment

Choose a reason for hiding this comment

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

This is good, thanks. The files you modified are for a large part auto-generated or copied from another directory. Can you modify the source files in krmllib/c/{fstar_uint128*.h} rather? Then I can take care of refreshing dist after that and check that it's alright. Thanks!

@@ -25,7 +25,7 @@

#include "FStar_UInt128.h"
#include "FStar_UInt_8_16_32_64.h"
#include "lowstar_endianness.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

this one seems wrong

@@ -2,4 +2,4 @@ USER_TARGET=libkrmllib.a
USER_CFLAGS=
USER_C_FILES=fstar_uint128.c
ALL_C_FILES=
ALL_H_FILES=FStar_UInt128.h FStar_UInt_8_16_32_64.h lowstar_endianness.h
Copy link
Contributor

Choose a reason for hiding this comment

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

same here should be the builtin one

@@ -25,7 +25,7 @@

#include "FStar_UInt128.h"
#include "FStar_UInt_8_16_32_64.h"
#include "lowstar_endianness.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

same

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.

2 participants