Lambosaurus / py-c-preprocessor

Tools for preprocessing C files in python
MIT License
15 stars 3 forks source link

Added variadic parameter support #13

Closed NotYourFox closed 1 month ago

NotYourFox commented 1 month ago

Changes since previous (cancelled, which I'm now glad it was) pull request:

The code seems to pass all the tests. It is highly recommended to review the new tests and features to ensure full coverage of all the possible cases and full backwards compatibility with the upstream version.

Lambosaurus commented 1 month ago

I would be careful what changes you actually commit in future - and would recommend you review your changes as you stage them - making sure you are only committing lines that you intended to touch. This makes it both easier to review, and make sure your not making any mistakes.

For example, you've committed ignore_macro_definitions. I cant tell if this is a feature or merely test code that got accidentally included.

The reason why all the lines are marked as changed is probably due to the line-endings getting changed. I probably started this project on a windows machine.

Normally I wouldn't want to include any test code in the main branch for a feature that doesn't exist in main. You'd be better off writing those tests in a feature branch, and only merge the tests in once they are associated with a working feature. Ensuring that a PR is associated with only the features you want to implement makes it less likely that a crabby old engineer will have lots of nit-picks.

I'll check this over tomorrow.

NotYourFox commented 1 month ago

The ignore_macro_definitions thing was here from the very beginning and I kinda forgot about it. The reason for including it for me was that it can be an annoyance when the code doesn't work if it encounters a single macro it doesn't support, so I guessed it'd be better to explicitly mark certain macros to be ignored. I did think of documenting and reviewing this, but then I just forgot I ever did that, so I guess you can consider it "test code". I will probably revert that for now.

And I did reconfigure my editor to preserve line endings to not make things messy in the future, thanks for explaining, I didn't quite think of that.

Lambosaurus commented 1 month ago

Cool. I'm going to approve this.

I think in general - there's a bit of a philosophical issue of when to raise errors here. Errors are raised when expanding a macro with varargs if the varags have been declared out of order, ie,macro(a, ..., b) I feel like this should be raised when the macro is defined, instead of when it is expanded. The expansion/substitution logic should be able to rely on having a valid macro. I may rework some of this later (when I'm not sick....).

NotYourFox commented 1 month ago

Yeah, I guess you're right. I suppose a solution would be to include vname as a field in Macro (None if not a variadic), and just move the implementation to Preprocessor.define

I've also noticed that you have this _directive_define_varidic method which handles function-like macro definitions, so I guess it should be called something like _directive_define_function_like

# Rule to handle: #define <token>(<any>) [<expression>]
def _directive_define_function_like(self, args):
    params = [ a.strip() for a in args[1].split(",") ]
    self.define(args[0], args[2], params)
NotYourFox commented 1 month ago

In addition: As I started playing around with the proposed solution, I encountered an issue where a definition of a function-like macro with no parameters was treated by the code as invalid. At first I thought that an empty argument can simply be stripped out, however I didn't like making this case identical to a constant macro definition. So I changed this:

# elif re.search(TOKEN_SEARCH_REGEX, args[i]) is None
elif re.search(TOKEN_SEARCH_REGEX, args[i]) is None and not (len(args) == 1 and re.match(r'\s*', args[0]) is not None)

which I consider a stub. The problem is that the code does not actually distinguish between constant and function-like macros, which I've addressed in issue #14, with the key difference between

#define MACRO_C x
// and
#define MACRO_F() x

trivially derived from it.

Fixing the issue could, aside from dealing with unexpected errors, help handling function-macros with no parameters correctly. So, perhaps, removing an empty argument would become a viable and a cleaner option, granted that the macro in question would be acknowledged as being function-like and treated that way further.

Lambosaurus commented 1 month ago

Issue of macro functions with zero arguments is started here: #15