-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
RFC: Cleaning up macro naming, possibly switch to inline functions #293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
CAPS are good because they tell you it's a macro. I would rather make something a function than keep it a macro with lower case letters. Are C99 inline functions mature/good enough to replace macros? (At least macros that don't use ## etc). |
But why anyone would bother to know it's a macro? The only reason, as mentioned in previous discussion, would be "caution, this thing may eval args with side-effects twice", but then macro should be written to be free from this double-eval (and most in uPy are likely already are).
With GCC, depending on optimization level, you may get inline functions not being inlined, which may be a feature or misfeature. To get 100% inlinability, one must use |
Okay, I'm happy to inline the MP_OBJ_IS_* functions. For MP_OBJ_NEW_SMALL_INT, what if it's one day used to initialise static const data? Eg a static const list of uPy objects, some of which are strs, some ints? |
Good question, worth trying, but as C++11 introduced "constexpr" modifier for that to work, I guess it's not going to work with inline functions. That means we should seriously consider just downcasing, and leave inlines for cool new usages, when such cool new usages arise ;-). |
Okay, done some testing with macros vs inline functions. I considered the 3 "functions" MP_OBJ_IS_TYPE, MP_OBJ_IS_INT and MP_OBJ_IS_STR. I tried different ways of compiling them, and got the following binary size for stmhal:
So, it's almost 1500 bytes extra to inline, but only 16 bytes extra to make macros! |
Pretty weird and something unright there. Dunno what. Switching to plain inlines also broke DEBUG=1 build. Using static inlines fixes that. |
Yeah, I don't trust/understand the C compiler with it's use of inline. The static inline I understand: see eg http://stackoverflow.com/questions/216510/extern-inline/216546#216546 |
I mentioned in some other comment that looking at some code for some port I got an impression that compiler may generate per-object instance of (non-inlined) inline function, i.e. the function body may be repeated multiple times in a final executable. I just got reminded about it while reading "man gcov":
So, what I saw is linker not removing other copies. Let this be anecdotal experience for now, but something to check on the next close look at the issue. |
Is it possible this has to do with compiler/linker options? Maybe one needs to tell gcc optimize for size or so? I just tried the same with msvc (replace MP_OBJ_IS_TYPE, MP_OBJ_IS_INT and MP_OBJ_IS_STR with normal inline functions), and the executable size stays exactly the same since the generated assembly is also exactly the same. Which is sort of logical, for such simple functions you'd expect the optimizer can see through the function calls and just replace it with bare instructions. |
I use the I just ran into an issue where I tried to use inline function definitions and it wouldn't compile under the unix port. Are inline functions completely turned off? They definitely have uses! If I change this line in both the header and C file:
to
The compiler fails with this error
Are inline functions turned off? |
@cloudformdesign you need to write |
no! what I needed was to declare it as See: http://www.greenend.org.uk/rjk/tech/inline.html looking at this reference more, I think that both the |
I tried again to convert macros to inline functions. Using gcc 5.2.0. I can get a decrease of around 500 bytes on 64-bit unix port when using inline functions, and the same size (+/- 10 bytes) for stmhal. Everything except MP_OBJ_IS_TYPE is inlined, since inlining this adds around 500 bytes to stmhal. Also, we can't inline MP_OBJ_NEW_SMALL_INT and MP_OBJ_NEW_QSTR because they are used to initialise constant data it many places. So maybe we can downcase everything except those 2 above that must remain macros to initialise static data? And inline everything except MP_OBJ_IS_TYPE (which would become mp_obj_is_type)? |
Downcase - sure. But to inline, relying on novel gcc version doesn't sound like a good idea. Nobody uses 5.2.0 currently and it will be few years till whole world starts to use it. So, if going back to inlining ideas, it would be good to understand what went right in the previous attempt, to be able to control the situation. (And I don't think it's a priority - there're lot of other things to do.) |
Yeah, I also don't like how inconsistent it is. Anyway, main thing is that we have a plan to eventually work towards -- ie downcase and inline. |
This is pretty much addressed by #4446, which converted most macros/inline functions to lowercase. |
This is to capture #269 (comment) and following discussion.
Summary:
The text was updated successfully, but these errors were encountered: