carld / micro-lisp

πŸŽ„A very small Lisp programming language πŸ˜€that used to be under 200 lines of CπŸŽ„
MIT License
798 stars 69 forks source link

Parenthesize variable use inside macros #4

Closed ghost closed 6 years ago

ghost commented 6 years ago
#define is_space(x)  (x == ' ' || x == '\n')
#define is_parens(x) (x == '(' || x == ')')

Should be

#define is_space(x)  ((x) == ' ' || (x) == '\n')
#define is_parens(x) ((x) == '(' || (x) == ')')

Otherwise, C's operator precedence applies for the == operator and whatever operations x evaluates to.

carld commented 6 years ago

Yeah good points πŸ‘

xyproto commented 6 years ago

With 87fda411680f762381d033153dd3eacb57a10f31 (making the macros into functions) this issue should be resolved.

ghost commented 6 years ago

Well actually... You seem to not understand the problem. https://stackoverflow.com/questions/10820340/the-need-for-parentheses-in-macros-in-c

#define is_pair(x) (((uintptr_t)x & 0x1) == 0x1)
#define is_atom(x) (((uintptr_t)x & 0x1) == 0x0)
#define untag(x)   ((uintptr_t) x & ~0x1)
#define tag(x)     ((uintptr_t) x | 0x1)
xyproto commented 6 years ago

@mar77i Really? The macros are no longer there, in the code.

Are you saying the problem with parenthesis in macros also apply to the regular C functions?

carld commented 6 years ago

It's a good safety tip to parenthesize macro parameters. Running gcc -E to get preprocessor output, I noticed one place in micro-lisp.c where a macro parameter captures an operator in the expansion: args = &( (List *) ((uintptr_t) *args & ~0x1) )->next;. There the dereference operator * takes higher precedence than the bitwise & and it would seem that line still does what is intended after macro expansion. It would be great to see a PR if anyone wants to contribute further!

ghost commented 6 years ago

Responding because @xyproto left it unclear whether my reasoning to reopen was sound. As of 3333b0d the four macros I listed in my second comment remain in the code unparenthesized. Eg. is_pair(0xc | 0x20) creates (((uintptr_t)0xc | 0x20 & 0x1) == **0x1). Gcc should display a warning in cases like these.