abiggerhammer / hammer Goto Github PK

Currently, the naming conventions for the action and validation functions are:

act_<rulename>
validate_<rulename>

One is an abbreviated form, one a full form, and the user must remember which case uses which. It's probably more boring, but IMO easier to remember (and hence less likely to get wrong) if both would use either the full, or both the abbreviated form. I.e. either

action_<rulename>
validate_<rulename>

or

act_<rulename>
val_<rulename>

Looking forward to your thoughts.

SCons barfs if you don't have pkg-config. Hence, it's not an "optional" dependency.

parser = Hammer::ParserBuilder.new
NameError: uninitialized constant Hammer::ParserBuilder

In the provided examples, H_ALLOC and friends could fail. Provide a way to bail out (we might have to abandon the parse - full recognition is not possible without the memory).

TQ on IRC suggested a longjmp to the beginning of the parse, which I like, however a nicer way to abort the parse might be handy...

After a successful scons build, scons test works if, and only if, I have done a scons install before building the test target. IMO, this defeats the purpose of the test target, since I should test it before installing, and install only after all tests yielded "ok".

A couplefew things:

• If users are expected to use the TT_* names everywhere else, it makes sense for them to use the TT_* names when calling H_MAKE. Currently, the H_MAKE macro adds the TT_ prefix on its own. I expect this is because the implementation of the underlying h_make function expects itself to be used only for user-defined token types, thus shielding the user. Given the nascent token-type registry, however, I believe this is destined to (and should) be changed.
• I understand that folding the TT_ prefix into the macro body is intended to make the H_MAKE_SEQ, etc macros look more or less the same, but I still think it's confusing to use the TT_ some places but not others.
• Also given the nascent token-type registry, I think it makes sense to provide an another wrapper for h_make that interacts with it. Perhaps an "H_MAKE_USER" macro would do the trick, taking a string and doing the registry lookup within. Though perhaps a full-on function might be better.
• That said, were I allowed to be super-greedy, I'd love to see exactly one token-creation entry point that expects an int, and exactly one wrapper that expects a string and converts it to an int via a registry lookup. That said, I totally see how it's more efficient to do it this way (though I think it might be worth at least investigating how much prettier the code is vs. how much slower it performs). What I wouldn't give for pattern matching in C!

I'm happy to write code for any/all of this, but I don't want to start digging in without some discussion/approval from the powers-that-be. =)

$ make -j1
for dir in src examples; do make -C ${dir} all; done
CC=cc
make[1]: Entering directory `/build/hammer/src'

• Compiling bitreader.c... [!!]
  cc1: warnings being treated as errors
  bitreader.c: In function ‘test_bitreader_ints’:
  bitreader.c:126: error: implicit declaration of function ‘g_test_fail’
  make[1]: *** [bitreader.o] Error 1
  make[1]: Leaving directory /build/hammer/src' CC=cc make[1]: Entering directory/build/hammer/examples'
• Compiling dns.c... [OK]
• Linking dns... [!!]
  /usr/bin/ld: cannot find -lhammer
  collect2: ld returned 1 exit status
  make[1]: *** [dns] Error 1
  make[1]: Leaving directory `/build/hammer/examples'
  make: *** [all] Error 2

In particular:

$ cd src && make -j1
CC=cc

• Compiling bitreader.c... [!!]
  cc1: warnings being treated as errors
  bitreader.c: In function ‘test_bitreader_ints’:
  bitreader.c:126: error: implicit declaration of function ‘g_test_fail’
  make: *** [bitreader.o] Error 1

More information:

$ a show libglib2.0-dev | grep Version
Version: 2.24.2-1

According to [0], the g_test_fail function isn't in glib until 2.29.4, so I don't know how likely this is to ever work, but it should probably be documented if nothing else.

Many thanks,
--frozencemetery

[0] : https://mail.gnome.org/archives/gtk-devel-list/2011-May/msg00012.html

If a token is a prefix of another token in a choice, then depending on the order of their listing in the choice influences the results of the parser. an example:

#include <hammer/hammer.h>
#include <stdio.h>
#include <string.h>

// test with
// gcc -DBUG -o prefixbug prefixbug.c $(pkg-config --cflags --libs libhammer)
// echo "asdfv" | ./prefixbug # should return yay, does boo
// c.f.
// gcc -o prefixbug prefixbug.c $(pkg-config --cflags --libs libhammer)
// echo "asdfv" | ./prefixbug # correctly^Was expected returns yay

int main(int argc, char *argv[]) {
    uint8_t input[1024];
    size_t inputsize;

    HParser *eol = h_token("\n",1);
    HParser *words = h_choice(
#ifdef BUG
                              h_token("asdf",strlen("asdf")),
                              h_token("asdfv",strlen("asdfv")),
#else // no bug
                              h_token("asdfv",strlen("asdfv")),
                              h_token("asdf",strlen("asdf")),
#endif
                              NULL);
    HParser *rule = h_sequence(words,eol,NULL);

    inputsize = fread(input, 1, sizeof(input), stdin);

    HParseResult *result = h_parse(rule, input, inputsize);
    if(result) {
        printf("yay!\n");
    } else {
        printf("boo!\n");
    }
}

sorry if this is due to my poor understanding and in fact an intended feature.

LL(k) doesn't handle parses that don't consume all their input. We need to make an executive decision on what to do about that; for the meantime, the test cases that were causing test_many and test_many1 to fail have been modified to ones that don't have trailing input.