Overskrift

I C deler alle enum navn det samme navneområdet med hverandre ( og med ting som variable navn). Det er derfor en god ide å prøve å redusere risikoen for at de kolliderer.

Dine enum json_value_type navn har prefikset TYPE_, som er ganske generisk. Noen andre biblioteker kan prøve å bruke samme navn. Jeg foreslår at du endrer prefikset til, si, JSON_.

Du ser heller ikke ut til å bruke TYPE_KEY for hva som helst. Bare fjern den.

Implementering

Som Roland Illig bemerker , argumentene til iscntrl() og isspace() i skip_whitespace() -funksjonen skal kastes til unsigned char til unngå skiltforlengelse.

Alternativt, og nærmere følger JSON-spesifikasjonen , kan du omskrive denne funksjonen ganske enkelt som:

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

Mange av dine static hjelperfunksjoner gjør ikke-trivielle kombinasjoner av ting, og mangler noen kommentar som forklarer hva de gjøre. Én eller to kommentarlinjer før hver funksjon kan hjelpe lesbarheten mye.

Spesielt gjør has_char() -funksjonen en rekke forskjellige ting:

1. Den hopper over mellomrom.
2. Den sjekker for tilstedeværelsen av et bestemt tegn i inngangen.
3. Hvis tegnet blir funnet, hopper det automatisk over det.

Bare nr. 2 er åpenbart underforstått av funksjonsnavnet; de andre er uventede bivirkninger, og bør i det minste være tydelig dokumentert.

Egentlig ser det ut til at det kan være bedre å fjerne samtalen til skip_whitespace() fra has_char(), og bare la innringeren eksplisitt hoppe over mellomrom før du ringer til det om nødvendig. I mange tilfeller gjør koden din det allerede, noe som gjør at duplikatet hopper overflødig.

Også, for å gjøre effekt nr. 3 mindre overraskende for leseren, kan det være lurt å gi den funksjonen nytt navn til noe litt mer aktiv som, si, read_char().

På slutten av json_parse_object() har du:

 return success; return 1; } 

Sikkert det er overflødig. Bare kvitt return 1;.

Også det ser ut til at du bruker den generiske json_parse_value() -funksjonen for å analysere objektnøkler, og ikke teste for å sikre at de er strenger. Dette gjør at noen ugyldige JSON kan komme gjennom analysatoren din. Jeg foreslår at du enten legger til en eksplisitt typekontroll eller deler strengekodekoden i en egen funksjon (som beskrevet nedenfor) og kaller 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åte som du gjør i json_parse_object():

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

(Bare du vet, jeg tror fortsatt navnet read_char() ville være bedre.)

Av en eller annen grunn ser det ut til at json_parse_array() forventer at den som ringer vil initialisere parent struct, mens json_parse_object() gjør det automatisk. AFAICT det er ingen grunn til inkonsekvensen, så du kan og burde sannsynligvis bare få begge funksjonene til å fungere samme måte.

json_is_literal() -funksjonen din er ikke merket som static, selv om den ikke gjør det vises i overskriften. Som is_char(), hadde jeg al så foretrekk å endre navn på det til noe mer aktivt, som json_read_literal() eller bare read_literal(), for å gjøre det tydeligere at det automatisk flytter markøren på en vellykket kamp.

(Vær også oppmerksom på at, som skrevet, funksjonen ikke kontrollerer at bokstavene i inngangen faktisk slutter der den skal. For eksempel ville det lykkes å matche inngangen nullnullnull mot null.Jeg tror ikke det er en faktisk feil, siden de eneste gyldige bokstavene i JSON er true, false og null, hvorav ingen er prefikser av hverandre, og siden to bokstaver ikke kan vises fortløpende i gyldig JSON uten noe annet symbol i mellom. Men det er i det minste i det minste verdt å merke seg i en kommentar.)

Du vil kanskje også eksplisitt merke noen av de statiske hjelperfunksjonene dine som inline å gi kompilatoren et hint om at den skal prøve å slå dem sammen i ringekoden. Jeg foreslår at du gjør det i det minste for skip_whitespace(), has_char() og json_is_literal().

Siden json_value_to_X() -tilbehørsfunksjonene består av ingenting annet enn en assert() og en pekereferanse, bør du også vurdere å flytte implementeringene til json.h og merke dem som static inline. Dette vil tillate kompilatoren å integrere dem i ringekoden selv i andre .c filer, og muligens optimalisere bort assert() hvis samtalen koden sjekker allerede typen uansett.

I hoved json_parse() -funksjonen vil du kanskje eksplisitt kontrollere at det ikke er annet enn hvitt mellomrom igjen i input etter at rotverdien er blitt analysert.

Strengparsering

Strengparseringskoden i json_parse_value() er ødelagt, siden den ikke «t håndterer tilbakeslag. Det mislykkes for eksempel på følgende gyldige JSON-inngang:

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

Det kan være lurt å legge til det som en testtilfelle.

Du bør også teste at koden din håndterer andre backslash escape-sekvenser som \b, \f, \n, \r, \t, \/ og spesielt \\ og \unnnn. Her er noen få testtilfeller for de:

"\"\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" 

Siden JSON-strenger kan inneholde vilkårlige Unicode-tegn, må du bestemme hvordan du skal håndtere dem. Sannsynligvis vil det enkleste valget være å erklære at input og output er i UTF-8 (eller kanskje WTF-8 ) og å konvertere \unnnn rømmer inn i UTF-8 bytesekvenser (og eventuelt omvendt). Vær oppmerksom på at siden du bruker nullterminerte strenger, kan det være at du foretrekker å avkode \u0000 til den overlange kodingen "\xC0\x80" i stedet for en nullbyte.

For å holde hoved json_parse_value() -funksjonen lesbar, Jeg vil på det sterkeste anbefale å dele strengeparsing-koden i en egen hjelperfunksjon. Spesielt fordi det å gjøre det til å håndtere tilbakeslag slipper riktig vil komplisere det betydelig.

En av komplikasjonene er at du ikke faktisk vet hvor lenge streng vil være til du har analysert den. En måte å håndtere det på ville være å dynamisk utvide den tildelte utgangsstrengen med realloc(), f.eks. slik:

// 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 vil være å kjøre to parseringskort over inngangen: en bare for å bestemme lengden på utgangsstrengen, og en annen for å faktisk dekode den. Dette vil lettest gjøres noe sånt 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; } 

der hjelperfunksjonen:

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

analyserer en JSON-streng på maksimalt *length bytes i new_string og skriver den faktiske lengden på den analyserte strengen i *length, eller, Hvis new_string == NULL, bare bestemmer lengden på strengen uten å lagre den dekodede utgangen noe sted.

Antall analyse

Din nåværende json_parse_value() implementering behandler tall som standardtilfelle, og rett og slett mater alt som ikke er med ", [, {, n, t eller f i C-standardbiblioteksfunksjonen strtod().

Siden strtod() godtar et supersett av gyldige JSON-nummerbokstaver, dette skal fungere, men kan gjøre koden din noen ganger cept ugyldig JSON som gyldig. Koden din vil for eksempel godta +nan, -nan, +inf og -inf som gyldige tall, og vil også godta heksadesimal notasjon som 0xABC123. Som strtod() -dokumentasjonen som er lenket ovenfor, merkes også:

I et annet sted enn standard «C» eller «POSIX» lokaler, kan denne funksjonen gjenkjenne ytterligere lokalavhengig syntaks.

Hvis du vil være strengere, vil du kanskje eksplisitt validere alt som ser ut som et tall mot JSON-grammatikk før du sender den til strtod().

Vær også oppmerksom på at strtod() kan angi errno f.eks hvis inngangsnummeret er utenfor området til en double. Du burde sannsynligvis se etter dette.

Testing

Jeg har ikke sett på testene dine i detalj, men det er flott å se at du har dem (selv om, som nevnt ovenfor kan dekningen deres forbedres).

Personlig vil jeg imidlertid foretrekke å flytte testene ut av implementeringen til en egen kildefil. Dette har både fordeler og ulemper:

• Den største ulempen er at du ikke lenger kan teste statiske hjelperfunksjoner direkte. Men med tanke på at det offentlige API-et ditt ser rent og omfattende ut, og ikke lider av problemer med «skjult tilstand» som kan komplisere testingen, bør du kunne oppnå god testdekning selv bare gjennom API-en.
• Den største fordelen (i tillegg til en ren skille mellom implementering og testkode) er at testene dine automatisk tester den offentlige API-en. Spesielt vil eventuelle problemer med json.h topp vises i testene dine. Også å gjøre testene dine via API hjelper deg med å sikre at API-et virkelig er tilstrekkelig komplett og fleksibelt for generell bruk.

Hvis du virkelig fortsatt vil teste dine statiske funksjoner direkte, du kan alltid legge til et forhåndsbehandlingsflagg som eventuelt utsetter dem for testing, enten via enkle innpakninger eller bare ved å fjerne nøkkelordet static fra definisjonene.

Ps. I la merke til at json_test_value_number() -testen mislyktes for meg (GCC 5.4.0, i386 arch), antagelig fordi tallet 23.4 ikke akkurat er representert i flytende punkt. Hvis du endrer den til 23.5, blir testen bestått.

Kommentarer

• Flott arbeid, takk, selvfølgelig gjorde jeg ikke ‘ t sjekk standarden så mye som jeg burde ha. Selv om jeg ikke så bort fra utf-8 med vilje (burde sannsynligvis ha nevnt det.
• OK, greit nok. Du burde nok i det minste fortsatt dekode enkle rømninger av skjemaet \u00XX siden noen kodere kan velge å bruke dem til og med for ASCII-tegn. (Også, jeg la til et forslag om å merke noen av funksjonene dine som inline ovenfor, siden jeg glemte å gjøre det tidligere .)
• Jeg savnet den delen av standarden helt, ja rømningssekvensene må analyseres riktig.

Dette er på ingen måte en fullstendig gjennomgang, men jeg vil dele noen ting som fanget meg når jeg leste koden din.

Kommentarer

Mens kommentarer er sikkert fine, noen av dine innebygde kommentarer legger bare til støy i koden.

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

Først og fremst er kommentaren en linje for tidlig. For det andre, man kan lese at det hvite området forbrukes ved å se på funksjonen – navnet beskriver den perfekt, th det er ikke behov for en ekstra kommentar.

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

Igjen, denne kommentaren gjentar bare hva selve koden sier.

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 }; 

Nå er disse kommentarene egentlig ikke ubrukelige siden de dokumenterer hva hver verdi representerer. Men hvorfor bare for TYPE_OBJECT og TYPE_ARRAY – hvorfor ikke for alle verdier? Personlig la jeg bare en lenke til json.org like før enum. Dine typer er analoge med de der, trenger du bare å dokumentere hva TYPE_KEY skal være. Hvilket bringer meg til neste punkt …

TYPE_KEY

Ser vi på json.org , kan du se et objekt består av en liste over medlemmer som igjen er laget av en streng og en verdi . Dette betyr at du ikke virkelig trenger TYPE_KEY! Bare legg til en ny struktur for medlemmer bestående av en TYPE_STRING -verdi og en annen json-verdi av hvilken som helst type, så er du klar. Nå kan du har f.eks. et tall som nøkkel for en verdi, som ikke er tillatt. Ville gjøre noe av den objektrelaterte logikken hyggeligere også, slik som for loop:

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

Ironisk nok kan trinnet i dette for loop faktisk bruke 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 noen få andre av dem. Jeg er ikke spesielt glad i å sette tilstand og uttalelse på samme linje, spesielt siden du ikke konsekvent gjør dette.Jeg er ganske greit med å gjøre dette for kontrollflyten, som if (!something) return;, men det er fortsatt «meh». Bedre gjør det riktig og legg utsagnet på en ny linje.

Også, jeg finner ut at koden din kan bruke noen flere tomme linjer for å skille «regioner» eller hva du vil kalle dem. For eksempel:

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; 

Det er en tom linje som skiller oppsett-og-analyser-ting fra innsjekk-og-retur-ting, men du kan gjø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 synes dette er mye renere. Du har en blokk for å sette opp verdiene, en blokk for å analysere dem, en blokk for å sette dem inn i vektoren, en blokk for å hoppe over mellomrom og en blokk for å fullføre den nåværende handlingen. Den siste tomme linjen mellom skip_whitespace(cursor); og if ... kan diskuteres , men jeg foretrekker det slik. ut fra det jeg har nevnt, er det ingenting jeg vil markere som uvanlig eller ikke-idomatisk.

Funksjonene fra ctype.h må ikke kalles med argumenter av typen char, siden det kan påberope seg udefinert oppførsel. Se NetBSD-dokumentasjonen for en god forklaring.