Overskrift

I C deler alle enum navne det samme navneområde med hinanden ( og med ting som variable navne). Det er derfor en god ide at forsøge at reducere risikoen for, at de kolliderer.

Dine enum json_value_type navne har præfikset TYPE_, som er ret generisk. Et andet bibliotek kan prøve at bruge det samme navn. Jeg foreslår at ændre dette præfiks til f.eks. JSON_.

Du ser heller ikke ud til at bruge TYPE_KEY til noget. Fjern det bare.

Implementering

Som Roland Illig bemærker , argumenterne for iscntrl() og isspace() i din skip_whitespace() -funktion skal kastes til unsigned char til undgå skiltudvidelse.

Alternativt og nøjere ved at følge JSON spec , kan du omskrive denne funktion simpelthen som:

static void skip_whitespace(const char** cursor) { while (**cursor == "\t" || **cursor == "\r" || **cursor == "\n" || **cursor == " ") ++(*cursor); } 

En masse af dine static hjælperfunktioner udfører ikke-trivielle kombinationer af ting og mangler nogen kommentar, der forklarer, hvad de gør. Én eller to kommentarlinjer før hver funktion kan hjælpe læsbarheden meget.

Især din has_char() -funktion gør en masse forskellige ting:

1. Det springer mellemrum over.
2. Det kontrollerer for tilstedeværelsen af et bestemt tegn i inputet.
3. Hvis tegnet findes, springer det automatisk over det.

Kun nr. 2 er tydeligt underforstået af funktionsnavnet; de andre er uventede bivirkninger og skal i det mindste være tydeligt dokumenterede.

Det ser faktisk ud til, at det kan være bedre at fjerne opkaldet til skip_whitespace() fra has_char(), og lad bare den, der ringer, springe det hvide mellemrum eksplicit ud, inden det kaldes, hvis det er nødvendigt. I mange tilfælde gør din kode det allerede, hvilket gør duplikatet springet overflødigt.

For at gøre effekt nr. 3 mindre overraskende for læseren kan det være en god ide at omdøbe den funktion til noget lidt mere aktiv som f.eks. read_char().

I slutningen af json_parse_object() har du:

 return success; return 1; } 

Det er bestemt overflødigt. Slip bare af med return 1;.

Også det ser ud til at du bruger den generiske json_parse_value() -funktion til at analysere objektnøgler og ikke teste for at sikre, at de er strenge. Dette gør det muligt for nogle ugyldige JSON at komme igennem din parser. Jeg foreslår enten at tilføje en eksplicit typekontrol eller opdele din strengparseringskode i en separat funktion (som beskrevet nedenfor) og kalde den direkte fra json_parse_object().

Øverst i json_parse_array() har du:

if (**cursor == "]") { ++(*cursor); return success; } while (success) { 

Du kan omskrive det på samme måde som du gør i json_parse_object():

while (success && !has_char("]")) { 

(Kun du ved, jeg tror stadig navnet read_char() ville være bedre.)

Af en eller anden grund synes din json_parse_array() at forvente, at den, der ringer op, initialiserer parent struct, mens json_parse_object() gør det automatisk. AFAICT der er ingen grund til inkonsekvensen, så du kunne og sandsynligvis bare skulle få begge funktioner til at fungere samme måde.

Din json_is_literal() -funktion er ikke markeret som static, selvom den ikke “t vises i overskriften. Ligesom is_char(), havde jeg al så foretrækker at omdøbe det til noget mere aktivt, som json_read_literal() eller bare read_literal(), for at gøre det tydeligere, at det automatisk fremfører markøren på en vellykket match.

(Bemærk også, at som skrevet, denne funktion ikke kontrollerer, at bogstavet i input faktisk slutter, hvor det skal. For eksempel ville det med succes matche input nullnullnull mod null.Jeg tror ikke, det er en egentlig fejl, da de eneste gyldige bogstaver i JSON er true, false og null, hvoraf ingen er præfikser for hinanden, og da to bogstaver ikke kan vises fortløbende i gyldig JSON uden noget andet token imellem. Men det er i det mindste i det mindste værd at bemærke i en kommentar.)

Du vil måske også eksplicit markere nogle af dine statiske hjælperfunktioner som inline for at give kompilatoren et antydning om, at den skal forsøge at flette dem i kaldekoden. Jeg foreslår at gøre det i det mindste for skip_whitespace(), has_char() og json_is_literal().

Da dine json_value_to_X() accessor-funktioner alle består af intet andet end en assert() og en pointerderference, bør du også overveje at flytte deres implementeringer til json.h og markere dem som static inline. Dette ville gøre det muligt for compileren at integrere dem i opkaldskoden selv i andre .c filer og muligvis optimere assert() hvis opkaldet kode kontrollerer allerede typen alligevel.

I din hoved json_parse() -funktion vil du muligvis eksplicit kontrollere, at der ikke er andet end hvidt mellemrum tilbage i input efter rodværdien er blevet parset.

Strengparsing

Din strengparseringskode i json_parse_value() er brudt, da den ikke “t håndter backslash undslipper. For eksempel mislykkes det med følgende gyldige JSON-input:

"I say: \"Hello, World!\"" 

Det kan være en god idé at tilføje det som en testsag.

Du bør også teste, at din kode korrekt håndterer andre backslash escape-sekvenser som \b, \f, \n, \r, \t, \/ og især \\ og \unnnn. Her er nogle få flere testtilfælde for dem:

"\"\b\f\n\r\t\/\\" "void main(void) {\r\n\tprintf(\"I say: \\\"Hello, World!\\\"\\n\");\r\n}" "\u0048\u0065\u006C\u006C\u006F\u002C\u0020\u0057\u006F\u0072\u006C\u0064\u0021" "\u3053\u3093\u306B\u3061\u306F\u4E16\u754C" 

Da JSON-strenge kan indeholde vilkårlige Unicode-tegn, skal du beslutte, hvordan du skal håndtere dem. Det enkleste valg ville sandsynligvis være at erklære din input og output for at være i UTF-8 (eller måske WTF-8 ) og konvertere \unnnn undslipper til UTF-8-bytesekvenser (og eventuelt omvendt). Bemærk, at da du bruger null-terminerede strenge, foretrækker du muligvis at afkode \u0000 til den overlange kodning "\xC0\x80" i stedet for en nullbyte.

For at holde hoved json_parse_value() læsbar, Jeg vil på det kraftigste anbefale at opdele streng-parsing-koden i en separat hjælperfunktion. Især da det at gøre det til at håndtere tilbageslag undslipper korrekt, vil det komplicere det betydeligt.

En af komplikationerne er, at du ikke faktisk ved, hvor længe streng vil være, indtil du har analyseret den. En måde at håndtere det på ville være at dynamisk vokse den tildelte outputstreng med realloc(), f.eks. sådan:

// resize output buffer *buffer to new_size bytes // return 1 on success, 0 on failure static int resize_buffer(char** buffer, size_t new_size) { char *new_buffer = realloc(*buffer, new_size); if (new_buffer) { *buffer = new_buffer; return 1; } else return 0; } // parse a JSON string value // expects the cursor to point after the initial double quote // return 1 on success, 0 on failure static int json_parse_string(const char** cursor, json_value* parent) { int success = 1; size_t length = 0, allocated = 8; // start with an 8-byte buffer char *new_string = malloc(allocated); if (!new_string) return 0; while (success && **cursor != """) { if (**cursor == "\0") { success = 0; // unterminated string } // we"re going to need at least one more byte of space while (success && length + 1 > allocated) { success = resize_buffer(&new_string, allocated *= 2); } if (!success) break; if (**cursor != "\\") { new_string[length++] = **cursor; // just copy normal bytes to output ++(*cursor); } else switch ((*cursor)[1]) { case "\\":new_string[length++] = "\\"; *cursor += 2; break; case "/": new_string[length++] = "/"; *cursor += 2; break; case """: new_string[length++] = """; *cursor += 2; break; case "b": new_string[length++] = "\b"; *cursor += 2; break; case "f": new_string[length++] = "\f"; *cursor += 2; break; case "n": new_string[length++] = "\n"; *cursor += 2; break; case "r": new_string[length++] = "\r"; *cursor += 2; break; case "t": new_string[length++] = "\t"; *cursor += 2; break; case "u": // TODO: handle Unicode escapes! (decode to UTF-8?) // note that this may require extending the buffer further default: success = 0; break; // invalid escape sequence } } success = success && resize_buffer(&new_string, length+1); if (!success) { free(new_string); return 0; } new_string[length] = "\0"; parent->type = TYPE_STRING; parent->value.string = new_string; ++(*cursor); // move cursor after final double quote return 1; } 

En alternativ løsning ville være at køre to parseringskørsler over input: en bare for at bestemme længden af outputstrengen, og en anden for faktisk at afkode den. Dette ville være mest let noget som dette:

static int json_parse_string(const char** cursor, json_value* parent) { char *tmp_cursor = *cursor; size_t length = (size_t)-1; if (!json_string_helper(&tmp_cursor, &length, NULL)) return 0; char *new_string = malloc(length); if (!new_string) return 0; if (!json_string_helper(&tmp_cursor, &length, new_string)) { free(new_string); return 0; } parent->type = TYPE_STRING; parent->value.string = new_string; *cursor = tmp_cursor; return 1; } 

hvor hjælperfunktionen:

static int json_parse_helper(const char** cursor, size_t* length, char* new_string) { // ... } 

analyserer en JSON-streng på højst *length bytes i new_string og skriver den faktiske længde af den parsede streng i *length, eller hvis new_string == NULL, bestemmer bare længden af strengen uden faktisk at gemme det dekodede output et vilkårligt sted.

Antal parsing

Din nuværende json_parse_value() implementering behandler tal som standardtilfælde og føder simpelthen alt, hvad der ikke er med ", [, {, n, t eller f i C-standardbiblioteksfunktionen strtod().

Da strtod() accepterer et supersæt med gyldige JSON-antal bogstaver, dette skal fungere, men kan gøre din kode undertiden til cept ugyldig JSON som gyldig. For eksempel accepterer din kode +nan, -nan, +inf og -inf som gyldige tal og accepterer også hexadecimal notation som 0xABC123. Som den ovenstående strtod() -dokumentation bemærker:

I et andet sprog end standard “C” eller “POSIX” lokaliteter, denne funktion genkender muligvis yderligere lokalafhængig syntaks.

Hvis du vil være strengere, kan du eksplicit validere alt, der ligner et tal, mod JSON-grammatik før den sendes til strtod().

Bemærk også, at strtod() muligvis indstiller errno f.eks hvis inputnummeret er uden for området double. Du burde sandsynligvis kontrollere dette.

Testning

Jeg har ikke kigget nærmere på dine tests, men det er dejligt at se, at du har dem (selvom, som nævnt ovenfor kunne deres dækning forbedres).

Personligt foretrækker jeg dog at flytte testene ud af implementeringen i en separat kildefil. Dette har både fordele og ulemper:

• Den største ulempe er, at du ikke længere direkte kan teste statiske hjælperfunktioner. Men i betragtning af at din offentlige API ser ren og omfattende ud og ikke lider af problemer med “skjult tilstand”, der vil komplicere testningen, bør du være i stand til at opnå god testdækning selv bare gennem APIen.
• Den største fordel (udover en ren adskillelse mellem implementering og testkode) er, at dine tests automatisk tester den offentlige API. Især vil eventuelle problemer med json.h header vises i dine tests. At lave dine tests via API hjælper dig også med at sikre, at din API virkelig er tilstrækkelig komplet og fleksibel til generel brug.

Hvis du virkelig stadig vil teste dine statiske funktioner direkte, du kan altid tilføje et præprocessorflag, der valgfrit udsætter dem til test, enten via enkle indpakninger eller bare ved at fjerne nøgleordet static fra deres definitioner.

Ps. I bemærkede, at din json_test_value_number() test mislykkedes for mig (GCC 5.4.0, i386 arch), formodentlig fordi tallet 23.4 ikke nøjagtigt kan repræsenteres i flydende punkt. At ændre det til 23.5 får testen til at bestå.

Kommentarer

• Fantastisk arbejde, tak, selvfølgelig gjorde jeg ikke ‘ t kontrollere standarden så meget som jeg skulle have. Selvom jeg tilsidesat utf-8 med vilje (burde sandsynligvis have nævnt det.
• OK, fair nok. Du skal sandsynligvis stadig mindst afkode enkle undslip af formularen \u00XX, da nogle kodere måske vælger at bruge dem selv til ASCII-tegn. (Også tilføjede jeg et forslag om at markere nogle af dine funktioner som inline ovenfor, da jeg glemte at gøre det tidligere .)
• Jeg savnede den del af standarden fuldstændigt, ja escape-sekvenserne skal analyseres korrekt.

Dette er på ingen måde en komplet gennemgang, men jeg vil dele nogle ting, der fangede mit øje, mens jeg læste din kode.

Kommentarer

Mens kommentarer er bestemt gode, nogle af dine integrerede kommentarer tilføjer kun støj til koden.

// Eat whitespace int success = 0; skip_whitespace(cursor); 

Først og fremmest er kommentaren en linje for tidligt. For det andet, man kan læse, at det hvide område forbruges ved at se på funktionen – navnet beskriver det perfekt, th det er ikke nødvendigt med en yderligere kommentar.

case "\0": // If parse_value is called with the cursor at the end of the string // that"s a failure success = 0; break; 

Igen gentager denne kommentar bare selve koden.

enum json_value_type { TYPE_NULL, TYPE_BOOL, TYPE_NUMBER, TYPE_OBJECT, // Is a vector with pairwise entries, key, value TYPE_ARRAY, // Is a vector, all entries are plain TYPE_STRING, TYPE_KEY }; 

Nu er disse kommentarer ikke rigtig ubrugelige, da de dokumenterer, hvad hver værdi repræsenterer. Men hvorfor kun til TYPE_OBJECT og TYPE_ARRAY – hvorfor ikke for alle værdier? Personligt ville jeg bare sætte et link til json.org lige før enum. Dine typer er analoge med dem der, skal du kun dokumentere, hvad TYPE_KEY skal være. Hvilket bringer mig til det næste punkt …

TYPE_KEY

Når du ser på json.org , kan du se et objekt består af en liste over medlemmer som igen er lavet af en streng og en værdi . Hvilket betyder, at du ikke virkelig har brug for TYPE_KEY! Bare tilføj en ny struktur til medlemmer bestående af en TYPE_STRING -værdi og en anden json-værdi af enhver type, og du er klar til at gå. Lige nu kan du har f.eks. et tal som nøgle til en værdi, som ikke er tilladt. Ville også gøre noget af den objektrelaterede logik pænere som denne for loop:

for (size_t i = 0; i < size; i += 2) 

Ironisk nok kunne trinnet i dette for loop faktisk bruge en kommentar (hvorfor += 2?) Men mangler en.

Diverse

case "\0": // If parse_value is called with the cursor at the end of the string // that"s a failure success = 0; break; 

Hvorfor ikke bare return 0;?

while (iscntrl(**cursor) || isspace(**cursor)) ++(*cursor); 

og

if (success) ++(*cursor); 

og

if (has_char(cursor, "}")) break; else if (has_char(cursor, ",")) continue; 

og et par andre af dem. Jeg er ikke især glad for at sætte tilstand og erklæring på samme linje, især da du ikke konsekvent gør dette.Jeg er okay med at gøre dette af hensyn til kontrolflow, ligesom if (!something) return;, men det er stadig “meh”. Bedre gør det rigtigt og sæt erklæringen på en ny linje.

Jeg finder også ud af, at din kode kunne bruge nogle flere tomme linjer til at adskille “regioner” eller hvad du end vil kalde dem F.eks .:

json_value key = { .type = TYPE_NULL }; json_value value = { .type = TYPE_NULL }; success = json_parse_value(cursor, &key); success = success && has_char(cursor, ":"); success = success && json_parse_value(cursor, &value); if (success) { vector_push_back(&result.value.object, &key); vector_push_back(&result.value.object, &value); } else { json_free_value(&key); break; } skip_whitespace(cursor); if (has_char(cursor, "}")) break; else if (has_char(cursor, ",")) continue; else success = 0; 

Der er en tom linje, der adskiller setup-and-parse-stuff fra check-and-return ting, men du kan gøre bedre.

json_value key = { .type = TYPE_NULL }; json_value value = { .type = TYPE_NULL }; success = json_parse_value(cursor, &key); success = success && has_char(cursor, ":"); success = success && json_parse_value(cursor, &value); if (success) { vector_push_back(&result.value.object, &key); vector_push_back(&result.value.object, &value); } else { json_free_value(&key); break; } skip_whitespace(cursor); if (has_char(cursor, "}")) break; else if (has_char(cursor, ",")) continue; else success = 0; 

Jeg finder dette meget renere. Du har en blok til at indstille værdierne, en blok til at parsere dem, en blok til at sætte dem ind i vektoren, en blok til at springe over mellemrum og en blok til at afslutte den aktuelle handling. Den sidste tomme linje mellem skip_whitespace(cursor); og if ... kan diskuteres , men jeg foretrækker det på denne måde.

Bortset fra det, fandt jeg din kode let læsbar og forståelig. Du kontrollerer korrekt for eventuelle fejl og bruger fornuftig navngivning. Hvad angår idiomatikken bortset fra ud fra det, jeg har nævnt, er der intet, som jeg markerer som usædvanligt eller ikke-idomatisk.

Funktionerne fra ctype.h må ikke kaldes med argumenter af typen char, da det kan påberåbe sig udefineret adfærd. Se NetBSD-dokumentationen for en god forklaring.