-
Notifications
You must be signed in to change notification settings - Fork 6
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
rm unused code #96
base: master
Are you sure you want to change the base?
rm unused code #96
Conversation
Ah, it is used, by the test suite (which apparently doesn't catch this, or runs in other ways) via
|
Well, it doesn't actually work anyway, because when this error hits, that diff --git a/serialization/testing/generic_suite.nim b/serialization/testing/generic_suite.nim
index 5f6df0a..3206f64 100644
--- a/serialization/testing/generic_suite.nim
+++ b/serialization/testing/generic_suite.nim
@@ -192,6 +192,7 @@ proc executeRoundtripTests*(Format: type) =
# If this doesn't work reliably, it will fail too silently.
# We need to report the number of checks passed.
when supports(Format, type(val)):
+ static: doAssert false
roundtripChecks(Format, val)
suite(name(Format) & " generic roundtrip tests"):
so it silently looks like it works, but doesn't, because of this failing For template supports*(_: type Json, T: type): bool =
# The JSON format should support every type
true But it's also a no-op of a call. And that appears to be 100% of the codebase usage of this particular |
To start with, it doesn't work right now:
in Nim 2.0.8 prints
false
, for example.status-im/nimbus-eth2#6573 (comment) has more context -- essentially
compiles( ... declval ...)
behaves a bit oddly at times.Nothing either in
nim-ssz-serialization
ornimbus-eth2
uses it, either.If people truly want this functionality, it can be reintroduced and reviewed as new code in a separate PR, but it's more important to remove the broken dependent on questionable assumptions which vary depending on all three of
(NimMajor, NimMinor, NimPatch)
.