Rubrik

I C delar alla enum namn samma namnområde med varandra ( och med saker som variabla namn). Det är därför en bra idé att försöka minska risken för att de kolliderar.

Dina enum json_value_type -namn har prefixet TYPE_, vilket är ganska generiskt. Något annat bibliotek kan försöka använda samma namn. Jag föreslår att du ändrar prefixet till, säg, JSON_.

Du verkar inte heller använda TYPE_KEY för vad som helst. Ta bara bort det.

Implementering

Som Roland Illig konstaterar , argumenten till iscntrl() och isspace() i din skip_whitespace() -funktion ska kastas till unsigned char till undvik teckenförlängning.

Alternativt och närmare följa JSON spec , kan du skriva om den här funktionen helt enkelt som:

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

Mycket av dina static hjälpfunktioner gör icke-triviala kombinationer av saker och saknar kommentar som förklarar vad de do. En eller två kommentarrader före varje funktion kan hjälpa till att läsa mycket.

I synnerhet gör din has_char() -funktion en massa olika saker:

1. Den hoppar över det vita utrymmet.
2. Den kontrollerar om det finns ett visst tecken i ingången.
3. Om tecknet hittas hoppar det automatiskt över det.

Endast # 2 antyds uppenbarligen av funktionsnamnet; de andra är oväntade biverkningar och bör åtminstone tydligt dokumenteras.

Det verkar faktiskt som om det är bättre att ta bort samtalet till skip_whitespace() från has_char(), och låt bara den som ringer uttryckligen hoppa över det vita utrymmet innan du ringer till det vid behov. I många fall gör din kod det redan, vilket gör att duplikatet hoppar överflödigt.

För att göra effekt nr 3 mindre överraskande för läsaren kan det vara en bra idé att byta namn på den funktionen till något lite mer aktiv som, säg, read_char().

I slutet av json_parse_object() har du:

 return success; return 1; } 

Visst är det överflödigt. Bli bara av med return 1;.

det ser ut som att du använder den generiska json_parse_value() -funktionen för att analysera objektnycklar och inte testa för att se till att de är strängar. Detta gör att vissa ogiltiga JSON kan komma igenom din parser. Jag föreslår antingen att du lägger till en uttrycklig typkontroll eller delar upp strängparsningskoden i en separat funktion (som beskrivs nedan) och ringer direkt från json_parse_object().

Överst på json_parse_array() har du:

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

Du kan skriva om på samma sätt som du gör i json_parse_object():

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

(Bara, du vet, jag tror fortfarande att namnet read_char() skulle vara bättre.)

Av någon anledning verkar din json_parse_array() förvänta sig att den som ringer initierar parent struct, medan json_parse_object() gör det automatiskt. AFAICT det finns ingen anledning till inkonsekvensen, så du kan och borde förmodligen bara få båda funktionerna att fungera samma sätt.

Din json_is_literal() -funktion är inte markerad som static, även om den inte gör det visas i rubriken. Precis som is_char(), jag hade al så föredra att byta namn på det till något mer aktivt, som json_read_literal() eller bara read_literal(), för att göra det tydligare att det automatiskt flyttar markören på en lyckad matchning.

(Observera också att, som skrivet, den här funktionen inte kontrollerar att bokstaven i ingången faktiskt slutar där den ska. Till exempel skulle det framgångsrikt matcha ingången nullnullnull mot null.Jag tror inte att det är ett verkligt fel, eftersom de enda giltiga bokstäverna i JSON är true, false och null, varav ingen är prefix av varandra, och eftersom två bokstäver inte kan visas i följd i giltig JSON utan någon annan symbol mellan. Men det är definitivt värt att notera i en kommentar.)

Du kanske också vill uttryckligen markera några av dina statiska hjälpfunktioner som inline för att ge kompilatorn en antydan om att den ska försöka slå dem i ringkoden. Jag föreslår att du gör det åtminstone för skip_whitespace(), has_char() och json_is_literal().

Eftersom dina json_value_to_X() accessorfunktioner består av allt annat än en assert() och en pekdereference, bör du också överväga att flytta deras implementeringar till json.h och markera dem som static inline. Detta skulle göra det möjligt för kompilatorn att infoga dem i anropskoden även i andra .c -filer och eventuellt optimera bort assert() om samtalet koden kontrollerar redan typen ändå.

I din huvudsakliga json_parse() -funktion, kanske du vill uttryckligen kontrollera att det inte finns något annat än vitt utrymme kvar i input efter att rotvärdet har analyserats.

Strängparsning

Din strängparsningskod i json_parse_value() är trasig eftersom den inte ”t hantera bakåtvänd snedstreck. Till exempel misslyckas det med följande giltiga JSON-ingång:

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

Du kanske vill lägga till det som ett testfall.

Du bör också testa att din kod hanterar korrekt andra backslash escape-sekvenser som \b, \f, \n, \r, \t, \/ och särskilt \\ och \unnnn. Här är några fler testfall för 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" 

Eftersom JSON-strängar kan innehålla godtyckliga Unicode-tecken måste du bestämma hur du ska hantera dem. Det enklaste valet är förmodligen att förklara att din inmatning och utdata är i UTF-8 (eller kanske WTF-8 ) och att konvertera \unnnn flyr till UTF-8 bytesekvenser (och eventuellt vice versa). Observera att eftersom du använder null-terminerade strängar kanske du föredrar att avkoda \u0000 till den överlånga kodningen "\xC0\x80" istället för en nollbyte.

För att hålla huvud json_parse_value() -läsbar, Jag rekommenderar starkt att dela strängparsningskoden i en separat hjälpfunktion. Särskilt eftersom det gör att det hanterar backslash-fläckar på rätt sätt kommer att komplicera det avsevärt.

En av komplikationerna är att du inte faktiskt vet hur länge sträng kommer att vara tills du har analyserat den. Ett sätt att hantera det skulle vara att dynamiskt växa den tilldelade utmatningssträngen med realloc(), t.ex. så här:

// 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 skulle vara att köra två parsningspass över ingången: en bara för att bestämma längden på utmatningssträngen och en annan för att faktiskt avkoda den. Detta skulle lättast kunna göras något så här:

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

där hjälpfunktionen:

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

tolkar en JSON-sträng på högst *length bytes i new_string och skriver den faktiska längden på den analyserade strängen i *length, eller, om new_string == NULL, bestämmer du bara strängens längd utan att faktiskt lagra den avkodade utmatningen någonstans.

Antal parsing

Din nuvarande json_parse_value() implementering behandlar siffror som standardfall och matar helt enkelt allt som inte är med ", [, {, n, t eller f i C-standardbiblioteksfunktionen strtod().

Eftersom strtod() accepterar ett superset av giltiga JSON-bokstäver, detta borde fungera, men kan göra din kod ibland ac cept ogiltig JSON som giltig. Till exempel accepterar din kod +nan, -nan, +inf och -inf som giltiga nummer och accepterar också hexadecimal notation som 0xABC123. Som strtod() -dokumentationen ovan länkade:

I ett annat land än standard ”C” eller ”POSIX” -lokaler kan den här funktionen känna igen ytterligare språkberoende syntax.

Om du vill vara strängare kan du uttryckligen validera allt som ser ut som ett tal mot JSON-grammatik innan den skickas till strtod().

Observera också att strtod() kan ställa in errno t.ex. om ingångsnumret ligger utanför intervallet för en double. Du borde nog se efter det här.

Testning

Jag har inte tittat på dina tester i detalj, men det är jättebra att se att du har dem (även om, som nämnts ovan skulle deras täckning kunna förbättras).

Personligen skulle jag dock vilja flytta testerna ur implementeringen till en separat källfil. Detta har både fördelar och nackdelar:

• Den största nackdelen är att du inte längre kan testa statiska hjälpfunktioner direkt. Med tanke på att ditt offentliga API ser rent och heltäckande ut och inte lider av några ”dolda tillstånd” -problem som skulle komplicera testningen, borde du kunna uppnå god testtäckning även bara via API.
• Den största fördelen (förutom en ren skillnad mellan implementering och testkod) är att dina tester automatiskt testar det offentliga API: et. Speciellt kommer eventuella problem med json.h rubriken att visas i dina tester. Att göra dina tester via API hjälper dig också att se till att ditt API verkligen är tillräckligt komplett och flexibelt för allmän användning.

Om du verkligen vill testa dina statiska funktioner direkt, Du kan alltid lägga till en förprocessorflagga som valfritt exponerar dem för testning, antingen via enkla omslag eller bara genom att ta bort sökordet static från deras definitioner.

Ps. I märkte att ditt json_test_value_number() -test misslyckades för mig (GCC 5.4.0, i386 arch), antagligen eftersom siffran 23.4 inte exakt kan representeras i flytande punkt. Att ändra den till 23.5 gör testet godkänt.

Kommentarer

• Bra jobbat, tack, självklart gjorde jag inte ’ t kontrollera standarden så mycket som jag borde ha. Även om jag avsiktligt ignorerade utf-8 (borde antagligen ha nämnt det.
• OK, rättvis. Du borde nog minst avkoda enkla undvikelser från formuläret \u00XX, eftersom vissa kodare kan välja att använda dem även för ASCII-tecken. (Dessutom lade jag till ett förslag att markera några av dina funktioner som inline ovan, eftersom jag glömde att göra det tidigare .)
• Jag saknade helt den delen av standarden, ja escape-sekvenser måste analyseras korrekt.

Detta är inte på något sätt en fullständig recension, men jag kommer att dela några saker som fångade mitt öga när jag läste din kod.

Kommentarer

Medan kommentarer är verkligen trevliga, några av dina inbyggda kommentarer lägger bara till buller i koden.

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

Först och främst är kommentaren en rad för tidigt. För det andra, man kan läsa att det vita utrymmet förbrukas genom att titta på funktionen – namnet beskriver det perfekt, th det finns inget behov av ytterligare en kommentar.

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

Återigen upprepar denna kommentar bara vad själva koden säger.

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 är dessa kommentarer inte riktigt värdelösa eftersom de dokumenterar vad varje värde representerar. Men varför bara för TYPE_OBJECT och TYPE_ARRAY – varför inte för alla värden? Personligen lade jag bara en länk till json.org precis innan enum. Dina typer är analoga med de där, du behöver bara dokumentera vad TYPE_KEY ska vara. Vilket tar mig till nästa punkt …

TYPE_KEY

Om du tittar på json.org kan du se ett objekt består av en lista med medlemmar , som i sin tur är gjorda av en sträng och ett värde . Vilket innebär att du inte verkligen behöver TYPE_KEY! Lägg bara till en ny struktur för medlemmar bestående av ett TYPE_STRING -värde och ett annat json-värde av vilken typ som helst och du är klar att gå. Just nu kan du har t.ex. ett tal som nyckel för ett värde, vilket inte är tillåtet. Skulle göra en del av den objektrelaterade logiken trevligare också, så här för loop:

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

Ironiskt nog kan steget för detta för loop faktiskt använda en kommentar (varför += 2?) Men saknar 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; 

Varför inte bara return 0;?

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

och

if (success) ++(*cursor); 

och

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

och några andra av dem. Jag är inte särskilt förtjust i att sätta villkor och uttalanden på samma linje, särskilt eftersom du inte konsekvent gör detta.Jag är okej med att göra detta för kontrollflödets skull, som if (!something) return;, men det är fortfarande ”meh”. Bättre gör det rätt och lägg uttalandet på en ny rad.

Jag tycker också att din kod kan använda några fler tomma rader för att separera ”regioner” eller vad du än vill kalla dem Till exempel:

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 finns en tom rad som skiljer installations-och-analysera-saker från kontroll-och-retur-saker, men du kan göra bättre.

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; 

Jag tycker att detta är mycket renare. Du har ett block för att ställa in värdena, ett block för att analysera dem, ett block för att sätta dem in i vektorn, ett block för att hoppa över tomt utrymme och ett block för att slutföra den aktuella åtgärden. Den sista tomma raden mellan skip_whitespace(cursor); och if ... kan diskuteras , men jag föredrar det på det här sättet.

Utöver det, tyckte jag att din kod var lättläsbar och förståelig. Du kontrollerar ordentligt för eventuella fel och använder förnuftiga namngivningar. från vad jag har nämnt finns det inget jag skulle markera som ovanligt eller oidomatiskt.

Funktionerna från ctype.h får inte anropas med argument av typen char, eftersom det kan åberopa odefinierat beteende. Se NetBSD-dokumentationen för en bra förklaring.