[hawkmoth] Re: [PATCH] parser: simplify macro arguments extraction

  • From: Jani Nikula <jani@xxxxxxxxxx>
  • To: Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx>, hawkmoth mailing list <hawkmoth@xxxxxxxxxxxxx>
  • Date: Fri, 11 Oct 2019 08:30:51 +0300

On Tue, 24 Sep 2019, Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx> wrote:

Previous solution required an extra dependency on itertools and it was
arguably uglier than what can be achieved in idiomatic Python. This
patch doesn't change behaviour in any way and it's purely stylistic.
---
Just a minor clean up I had laying around. Hope it's tasteful enough ;)

Thanks, and sorry for taking so long to get around to this!

Comments inline.


 hawkmoth/parser.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hawkmoth/parser.py b/hawkmoth/parser.py
index 9c1ec58..c4c137c 100644
--- a/hawkmoth/parser.py
+++ b/hawkmoth/parser.py
@@ -34,7 +34,6 @@
 """
 
 import enum
-import itertools
 import sys
 
 from clang.cindex import CursorKind, TypeKind
@@ -129,16 +128,16 @@ def _get_macro_args(cursor):
     if cursor.kind != CursorKind.MACRO_DEFINITION:
         return None
 
+    tokens = cursor.get_tokens()
+
     # Use the first two tokens to make sure this starts with 'IDENTIFIER('
-    x = [token for token in itertools.islice(cursor.get_tokens(), 2)]
-    if (len(x) != 2 or x[0].spelling != cursor.spelling or
-        x[1].spelling != '(' or x[0].extent.end != x[1].extent.start):
+    if next(tokens).spelling != cursor.spelling or next(tokens).spelling != 
'(':
         return None

First of all this approach looks much nicer in general than my clumsy
itertools thing.

However, this introduces two subtle bugs... should probably add test
cases for both of them.

1) #define FOO

If the macro has no value, the second next() will throw
StopIteration. This is the len(x) != 2 check in the original.

2) #define FOO (arg)

If the macro value starts with "(" the macro gets incorrectly identified
as a function-like macro. This is the x[0].extent.end !=
x[1].extent.start check in the original.

I like the use of next() here, but I think you'll have to use local
variables and either pass the default value to next() or wrap it in a
try block.


BR,
Jani.

 
     # Naïve parsing of macro arguments
     # FIXME: This doesn't handle GCC named vararg extension FOO(vararg...)
     args = []
-    for token in itertools.islice(cursor.get_tokens(), 2, None):
+    for token in tokens:
         if token.spelling == ')':
             return args
         elif token.spelling == ',':
-- 
2.23.0

Other related posts: