aradi / fypp

Python powered Fortran preprocessor
http://fypp.readthedocs.io
BSD 2-Clause "Simplified" License
188 stars 30 forks source link

Allow plain strings in defines #27

Open LecrisUT opened 1 year ago

LecrisUT commented 1 year ago

This allows to pass -DFOO=some_value

aradi commented 1 year ago

Thanks for the PR! I am somewhat unsure about this change, as it would change the user experience on typos. -DLOGICAL=false causes currently an error, while with you change, it would not generate an error, but instead create a variable LOGICAL which evaluates to True!

Is there any use case, where the additional quoting causes significant problems (apart of convenience)?

LecrisUT commented 1 year ago

I think the issue is that it counter to how the compilers are using define. There it is understood that -DSomething will create a definition of Something (as string), and not that it will evaluate a pythonic syntax. I believe for that case it is better to use a different flag like -P for parse.

As for the use-case, this will allow to transparently pass the pre-compile flags from one target to the other, e.g. in the cmake: https://github.com/aradi/fypp/pull/28/files#diff-596e5008f61a102e76c56394cda92747f836cce43620836bf97838608c482e24R272-R273 (i.e., these flags can be added/fixed after the definition of the target, which is common for nested cmake projects)

aradi commented 1 year ago

I agree, that it was probably counterintuitive to abuse -D to evaluate Python syntax and something like -P would have been a better choice (sorry!). However, changing it now would brake a lot of existing code, which I'd like to avoid.

What about an extra option to settle this? Something like -d or --define-value-type which controls how define values are interpreted, e.g. --define-value-type=expr (or -d expr) would evaluate it as Python expression, while --define-value-type=str (or -d str) as string literal. By keeping former as default, we would remain backwards compatible, while latter could be specified if no Python evaluation is required. (Actually, it maybe even enough just to use a single switch, like -s, which would turn the string interpretation on, when specified, as I don't think we will have other define value types as the current and the string one...)

LecrisUT commented 1 year ago

Indeed that would work. I would add though that there should be a deprecation message and transition for 3.3 or later where the default will change to the more natural syntax. Probably the documentation page should be improved and split for some time before that though so that it can be more visible there.

aradi commented 1 year ago

OK, let's do the following:

I'd need some time, though, to implement it.

LecrisUT commented 1 year ago

I've come back to add the discussed implementation. @aradi I find the python tests quite hard to follow, could you add the necessary tests? I'll try to clean them up later in #29 (or rather in a subsequent PR, so that one can get merged faster)