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.

Sign up for GitHub

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

Jump to bottom

Annotation arguments + small build fixes (depends on #44) #46

Merged
merged 3 commits into from
May 10, 2022
Merged

Annotation arguments + small build fixes (depends on #44) #46

merged 3 commits into from
May 10, 2022

Conversation

dtebbs
Copy link
Contributor

@dtebbs dtebbs commented Apr 19, 2022 •
edited
Loading

Along with some small build issues, fixes the mismatch between asserts on variable annotations (e.g. assert(annotation != "")) and the default parameters (e.g. annotation = "") which is highly confusing and can cause annoying errors.

Partially addresses issue #21.

@dtebbs dtebbs changed the title Fix annotation arguments Fix annotation arguments (depends on #44) Apr 20, 2022
@dtebbs dtebbs changed the base branch from develop to poly-commit-bdfg21 April 20, 2022 14:27
@dtebbs dtebbs marked this pull request as ready for review April 20, 2022 14:27
@dtebbs dtebbs requested a review from AntoineRondelet April 20, 2022 14:27
AntoineRondelet
AntoineRondelet approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@AntoineRondelet AntoineRondelet left a comment

Choose a reason for hiding this comment

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

These changes LGTM. I think enforcing variable annotation is a good idea (instead of being loose and checking them in debug mode only)
@dtebbs - what subtle errors are you referring to in your PR description?

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 21, 2022

@dtebbs - what subtle errors are you referring to in your PR description?

Namely that its possible to use some constructors / functions exactly as the interface allows, for the code to compile and run in some configuration, but then to later fail in another configuration with a cryptic error message (referring to a parameter not even specified). In general, we then need to see the callstack to find where the problem is.

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 21, 2022

The #if DEBUG around the assert also compounds the problem. Asserts are usually enabled by CMAKE_BUILD_TYPE=Debug, but for this particular assert we also needed the DEBUG build variable. (i.e. one may imagine that the assert should be enabled by simply doing a Debug build, but in fact this check was disabled, making it easy to miss empty annotations which cause a failure in other configs.

@AntoineRondelet
Copy link
Contributor

I see. Yes good point about surrounding the asserts by #ifdef DEBUG/#endif directives. I think we'd probably need to do another pass on the libraries to properly re-consider the use of the DEBUG cmake option. There may be other places where code in surrounded by such directives while it shouldn't be - strictly speaking.

@AntoineRondelet
Copy link
Contributor

@dtebbs shall I merge in #44? Also, is #44 ready for review? (It seems to be, but I haven't been assigned)

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 21, 2022 •
edited
Loading

I see. Yes good point about surrounding the asserts by #ifdef DEBUG/#endif directives. I think we'd probably need to do another pass on the libraries to properly re-consider the use of the DEBUG cmake option. There may be other places where code in surrounded by such directives while it shouldn't be - strictly speaking.

Yes, I think this would make sense at some point. AFAIK, DEBUG enables some checking functionality at the cost of execution time and memory. I think it should probably use a LIBSNARK_DEBUG or similar variable, instead of the very generic DEBUG option, but this should be confirmed when we do that second pass. I'll make an issue.

UPDATE: added a comment in #21

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 21, 2022

@dtebbs shall I merge in #44? Also, is #44 ready for review? (It seems to be, but I haven't been assigned)

Oh yes. Not sure why I didn't mark you for review. Thanks for spotting that.

@dtebbs dtebbs mentioned this pull request Apr 22, 2022
Merged
7 tasks
@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 25, 2022

Marking this WIP again. I'll combine with #47 to address the issues mentioned there.

@dtebbs dtebbs changed the title Fix annotation arguments (depends on #44) [WIP] Fix annotation arguments (depends on #44) Apr 25, 2022
@dtebbs dtebbs changed the title [WIP] Fix annotation arguments (depends on #44) Annotation arguments + small build fixes (depends on #44) Apr 25, 2022
@dtebbs dtebbs force-pushed the poly-commit-bdfg21 branch from 1fc3cb1 to a7b3303 Compare April 26, 2022 08:44
@dtebbs dtebbs force-pushed the annotation-fix branch from b5ef60d to 1db0855 Compare May 4, 2022 15:02
@AntoineRondelet AntoineRondelet self-requested a review May 4, 2022 15:33
AntoineRondelet
AntoineRondelet approved these changes May 4, 2022
View reviewed changes
Copy link
Contributor

@AntoineRondelet AntoineRondelet left a comment

Choose a reason for hiding this comment

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

LGTM - just a tiny comment typo to fix and I'll merge.

libsnark/gadgetlib1/gadget.tcc Outdated
@@ -16,6 +16,8 @@ gadget<FieldT>::gadget(
protoboard<FieldT> &pb, const std::string &annotation_prefix)
: pb(pb), annotation_prefix(annotation_prefix)
{
// Anotations may appear as "" (even if set by the calling code) unless
Copy link
Contributor

Choose a reason for hiding this comment

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

Annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Should be fixed.

@dtebbs dtebbs force-pushed the annotation-fix branch from 1db0855 to 4fe1fad Compare May 6, 2022 09:35
@AntoineRondelet AntoineRondelet changed the base branch from poly-commit-bdfg21 to develop May 10, 2022 15:32
AntoineRondelet
AntoineRondelet approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@AntoineRondelet AntoineRondelet left a comment

Choose a reason for hiding this comment

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

LGTM

@AntoineRondelet AntoineRondelet merged commit e94b7f2 into develop May 10, 2022
@AntoineRondelet AntoineRondelet deleted the annotation-fix branch May 31, 2022 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntoineRondelet AntoineRondelet AntoineRondelet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dtebbs @AntoineRondelet