Skip to content

Integer to string#31

Open
Aditya-132 wants to merge 3 commits intoschveiguy:masterfrom
Aditya-132:integertostring
Open

Integer to string#31
Aditya-132 wants to merge 3 commits intoschveiguy:masterfrom
Aditya-132:integertostring

Conversation

@Aditya-132
Copy link

@Aditya-132 Aditya-132 commented Apr 4, 2025

the error i encountering
(dmd-2.110.0)➜ jsoniopipe git:(integertostring) ✗ dub test
Generating test runner configuration 'jsoniopipe-test-library' for 'library' (library).
Starting Performing "unittest" build using /home/aditya/dlang/dmd-2.110.0/linux/bin64/dmd for x86_64.
Up-to-date iopipe 0.2.5: target for configuration [library] is up to date.
Building jsoniopipe ~integertostring: building configuration [jsoniopipe-test-library]
Linking jsoniopipe-test-library
Finished To force a rebuild of up-to-date targets, run again with --force
Running jsoniopipe-test-library
core.exception.AssertError@source/iopipe/json/serialize.d(114): unittest failure
??:? _d_unittestp [0x655315e0ad25]
source/iopipe/json/serialize.d:114 void iopipe.json.serialize.__unittest_L106_C1() [0x655315ddc409]
??:? void iopipe.json.serialize.__modtest() [0x655315dffb6e]
??:? int core.runtime.runModuleUnitTests().__foreachbody_L600_C5(object.ModuleInfo*) [0x655315e1bb6a]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda_L2459_C13(immutable(object.ModuleInfo*)) [0x655315e01d9b]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody_L584_C5(ref rt.sections_elf_shared.DSO) [0x655315e11503]
??:? int rt.sections_elf_shared.DSO.opApply(scope int delegate(ref rt.sections_elf_shared.DSO)) [0x655315e1168d]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) [0x655315e11491]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) [0x655315e01d6d]
??:? runModuleUnitTests [0x655315e1b99f]
??:? void rt.dmain2._d_run_main2(char[][], ulong, extern (C) int function(char[][])).runAll() [0x655315e0c95c]
??:? void rt.dmain2._d_run_main2(char[][], ulong, extern (C) int function(char[][])
).tryExec(scope void delegate()) [0x655315e0c8e9]
??:? _d_run_main2 [0x655315e0c852]
??:? _d_run_main [0x655315e0c63b]
/home/aditya/dlang/dmd-2.110.0/linux/bin64/../../src/druntime/import/core/internal/entrypoint.d:29 main [0x655315c9d8f1]
??:? [0x7d5156a2a1c9]
??:? __libc_start_main [0x7d5156a2a28a]
??:? _start [0x655315c9d7e4]
1/3 modules FAILED unittests
Error Program exited with code 1
(dmd-2.110.0)➜ jsoniopipe git:(integertostring) ✗


struct JSONValue(SType)
// Making JSONValue a template struct with a default parameter of string
struct JSONValue(StringType = string)
Copy link
Owner

@schveiguy schveiguy Apr 5, 2025

Choose a reason for hiding this comment

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

Just remove the template parameter. A template with a default value is somewhat useless in D, because you still would have to say JSONValue!()

Copy link
Author

Choose a reason for hiding this comment

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

source/iopipe/json/serialize.d(109,10): Error: template instance JSONValue!string JSONValue is not a template declaration, it is a struct this error was given when i made changes

StringType allocatedString;
}
// For SSO types, first byte can indicate length used
ubyte ssoLength;
Copy link
Owner

Choose a reason for hiding this comment

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

This is OK, for now. My suggestion was to use the type field to tell whether the length was used or not. I can fix this up later.

The goal is to maximize SSO size without adding to the size of the struct. In reality, I think we can pack this whole struct tighter.

case NumberSSO:
// Use std.conv.to for converting the char[] slice to StringType
import std.conv : to; // Add import here just to be safe/clear
return sso[0..ssoLength].to!StringType; // <-- Changed line
Copy link
Owner

@schveiguy schveiguy Apr 5, 2025

Choose a reason for hiding this comment

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

Returning the slice directly is preferred, as to!string allocates.

I have to think about the right types for this. returning a string is not good, because the SSO data inside is not immutable.

@schveiguy
Copy link
Owner

These are looking good, I'll have to look into the errors. Can't do it tonight, sorry.

Array,
Null,
Bool,
StringSSO, // Small string optimization for strings
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be possible to remove these from the public API? Whether SSO is used or not shouldn't matter to users, the SSO tag should be private to the JSONValue struct and access gated between getter functions.

Copy link
Author

Choose a reason for hiding this comment

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

ok i will remove it

@Inkrementator
Copy link
Contributor

These are looking good, I'll have to look into the errors. Can't do it tonight, sorry.

The errors were mostly just the enum names not having been updates in the tests, nothing bad

Aditya-132#1

here is the rebased version https://github.com/Inkrementator/jsoniopipe/tree/integertostring-patch2

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.

3 participants