search

we-like-parsers / pegen_experiments

Experiments for the official PEG parser generator for Python
https://github.com/python/cpython/tree/master/Tools/peg_generator
Other
273 stars 30 forks source link

Add CHECK_CALL to grammar and improve error checking code #225

Closed lysnikolaou closed 4 years ago

lysnikolaou commented 4 years ago

Closes #224.

lysnikolaou commented 4 years ago

If a helper returns NULL is always an error.

There are also the functions seq_{extract|delete}_starred_exprs, which might return NULL and that shouldn't be an error. What are we doing with those?

pablogsal commented 4 years ago

What are we doing with those?

Can we factor out the parts that return NULL for other reasons to the call site (if needed)? Maybe we need to enclose these functions in some helper that checks for error only on those or use another macro that also checks for PyErr_Occured().

lysnikolaou commented 4 years ago

@pablogsal Would something like 8cd74ed work?

Also, could you quickly test out if there's an unexpected performance regression on your system?

pablogsal commented 4 years ago

Also, could you quickly test out if there's an unexpected performance regression on your system?

On it

pablogsal commented 4 years ago

On it

Nothing that I can detect. Microbenchmarks for individual rules show some micro-seconds of difference for the extra check but that is expected. I checked what happens when compiled with PGO with perf (https://perf.wiki.kernel.org/index.php/Main_Page) and seems that the happy path branch is highly predictable so no problem.

lysnikolaou commented 4 years ago

On it

Nothing that I can detect. Microbenchmarks for individual rules show some micro-seconds of difference for the extra check but that is expected. I checked what happens when compiled with PGO with perf (https://perf.wiki.kernel.org/index.php/Main_Page) and seems that the happy path branch is highly predictable so no problem.

Nice.

Thanks a lot for the review!

gvanrossum commented 4 years ago

Sorry, I realize it's a bit late, since this has landed already. But for readability, I kind of think that the macro could reference p without it being passed in (since EXTRA does the same), and since it's pretty common, it could be named just CHECK().

lysnikolaou commented 4 years ago

Sorry, I realize it's a bit late, since this has landed already. But for readability, I kind of think that the macro could reference p without it being passed in (since EXTRA does the same), and since it's pretty common, it could be named just CHECK().

It's never too late. Opened #227.