Skip to content

Move to guarded includes #1022

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

Closed
dpgeorge opened this issue Dec 23, 2014 · 6 comments
Closed

Move to guarded includes #1022

dpgeorge opened this issue Dec 23, 2014 · 6 comments

Comments

@dpgeorge
Copy link
Member

It was inevitable that we would have to move away from Plan 9 include style to standard guarded includes :) It was good while it lasted, and lasted longer than expected.

Proposal for the change:

  1. Use macro name __MICROPY_INCLUDED_xxx_H__, and pattern:
#ifndef __MICROPY_INCLUDED_xxx_H__
#define __MICROPY_INCLUDED_xxx_H__
...
#endif // __MICROPY_INCLUDED_xxx_H__

where xxx is the filename in upper case without the .h. The leading and trailing double underscore are just my preference/habit and we may not want them.

  1. When #include'ing a uPy header we need to prefix it with the uPy directory, like: #include "py/obj.h". This is because we have very generic header names (misc.h, obj.h, bc.h, runtime.h, lexer.h) that can easily clash with other C code. By doing "py/obj.h" we don't need the py/ directory in the list of include search directories (we just need the root dir), and so the uPy headers won't clash with external headers.
  2. For consistency with (2), all .c files should use the uPy prefix directory when including a uPy header.
  3. We should probably rename the py/ directory to match the prefix chosen in mp_ prefix collision #704. Eg mpy/.
  4. We probably want a single, master header. Eg mpy/mpy.h, or mpy/micropy.h. This will just include all other headers.
@stinos
Copy link
Contributor

stinos commented Dec 23, 2014

Regarding the underscores: some would say it's reserved for system headers only but I'm not sure it's worth caring about. Also probably xxx shouldn't be just filename but rather DIRNAME_FILENAME to avoid conflicts (though that is maybe what you meant)

What would be the usecase for 4? It goes against the standard practice of including only what is strictly required (well, unless one requires everything of course), and is an extra maintainance burden.

@dpgeorge
Copy link
Member Author

Regarding the underscores: some would say it's reserved for system headers

I thought someone would mention this :) We don't need them, since MICROPY_INCLUDED_ is already a unique enough prefix for our code base.

Also probably xxx shouldn't be just filename but rather DIRNAME_FILENAME to avoid conflicts

Yes, good point. Having MICROPY in the macro already gives us a unique namespace, but we still need to distinguish extmod from py directories.

@dpgeorge
Copy link
Member Author

What would be the usecase for 4?

To make it easy for someone to embed uPy by included 1 header that has everything you need. Guided by Lua, we want the barrier to entry to embedding uPy to be as low as possible.

@stinos
Copy link
Contributor

stinos commented Dec 23, 2014

Makes sense.

@pfalcon
Copy link
Contributor

pfalcon commented Dec 23, 2014

There's some numbering issues. I guess we should go for 1,1,2 (i.e. first thress) asap. 4 is nice, yes. Renaming a dir can wait for decision until after #704 imho ;-).

stinos added a commit to stinos/micropython that referenced this issue Jan 1, 2015
dpgeorge added a commit that referenced this issue Jan 1, 2015
@dpgeorge
Copy link
Member Author

dpgeorge commented Jan 1, 2015

1, 1 and 2 have been fully implemented in a series of commits starting with 51dfcb4.

@dpgeorge dpgeorge closed this as completed Jan 1, 2015
stinos added a commit to stinos/micropython that referenced this issue Jan 2, 2015
- Use a single file env.props for defining the main directories used when building.
  env.props resolves the base directory and defines overridable output directories,
  and is used by all other build files.
- Fix the build currently failing, basically because the preprocessing command for generating
  qstrdefs uses different include directories than the build itself does.
  (specifically, qstrdefs.h uses #include "py/mpconfig.h" since the fixes for micropython#1022
  in 51dfcb4, so we need to use the base directory as include directory, not the py dir itself).
  So define a single variable containing the include directories instead and use it where needed.
tannewt pushed a commit to tannewt/circuitpython that referenced this issue Jul 13, 2018
nrf: Fix BLE on nRF52840 after adding the USB functionality
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants