[Monetdb-developers] [Monetdb-pf-checkins] pathfinder/compiler/algebra builtins.c, , 1.84, 1.85 logical.c, , 1.93, 1.94

Jan Rittinger rittinge at in.tum.de
Thu Apr 10 16:46:13 CEST 2008


Hi Jan,

your code looks quite good.

Picky as I am I however have some comments :)

Jan

On 04/10/2008 03:38 PM, Jan Flokstra wrote with possible deletions:
> [...]
> U builtins.c
> Index: builtins.c
> ===================================================================
> RCS file: /cvsroot/monetdb/pathfinder/compiler/algebra/builtins.c,v
> retrieving revision 1.84
> retrieving revision 1.85
> diff -u -d -r1.84 -r1.85
> --- builtins.c	3 Apr 2008 09:42:11 -0000	1.84
> +++ builtins.c	10 Apr 2008 13:38:38 -0000	1.85
> @@ -44,6 +44,7 @@
>  #include <assert.h>
>  #include <stdio.h>
>  
> +#include "mem.h"
              ^^^^^

what do you need that for?

> +
> +/*
> + * PFTIJAH defines. I decided not make a seperate include file for
> + * pftijah so ensure that these defines are exactly the same as in
> + * ../mil/milgen.brg
> + */

that is ugly in my eyes! -- You could place your stuff in algebra.h like 
e.g., the axis steps instead.

> +
> +#define MYNODEKIND  aat_pnode
> +#define DOCMGMTTYPE aat_docmgmt
> +
> +#define PFT_FUN(F)              (strncmp(F,"pftijah_",8)==0)
> +
> +#define PFT_QUERY_N_XX "pftijah_query_n_xx"
> +#define PFT_QUERY_N_SX "pftijah_query_n_sx"
> +#define PFT_QUERY_N_XO "pftijah_query_n_xo"
> +#define PFT_QUERY_N_SO "pftijah_query_n_so"
> +#define PFT_QUERY_I_XX "pftijah_query_i_xx"
> +#define PFT_QUERY_I_SX "pftijah_query_i_sx"
> +#define PFT_QUERY_I_XO "pftijah_query_i_xo"
> +#define PFT_QUERY_I_SO "pftijah_query_i_so"
> +
> +#define PTF_QUERY_NODES(N)      (N[14]=='n')
> +#define PTF_QUERY_STARTNODES(N) (N[16]=='s')
> +#define PTF_QUERY_OPTIONS(N)    (N[17]=='o')

Encoding all information in a single string is even more ugly!!!

You have a context (ctx) where you can store arbitrary information. Just 
build your own struct (in algebra.h) and make it less error-prone.

> +
> +PFalg_schema_t pft_empty_schema(PFalg_simple_type_t item_t) {
> +    PFalg_schema_t schema;
> +    schema.count = 3;
> +    // INCOMPLETE, what about freeing this space
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

we use garbage collection.

> +struct PFla_pair_t pft_query_param2(struct PFla_pair_t *p1, PFalg_simple_type_t itemType1, struct PFla_pair_t *p2, PFalg_simple_type_t itemType2) {
> [...]
> +struct PFla_pair_t pft_query_param3(struct PFla_pair_t *p1, PFalg_simple_type_t itemType1, struct PFla_pair_t *p2, PFalg_simple_type_t itemType2,  struct PFla_pair_t *p3, PFalg_simple_type_t itemType3) {

textwidth > 80

-- 
Jan Rittinger
Database Systems
Technische Universität München (Germany)
http://www-db.in.tum.de/~rittinge/




More information about the developers-list mailing list