crashappsec / libcon4m

Base Compiler and Runtime Support for con4m
Apache License 2.0
0 stars 0 forks source link

Remove private stuff from public hatrack headers #24

Closed orangematt closed 3 months ago

orangematt commented 3 months ago

This looks like a lot, but it's mostly just moving stuff around. Mainly it's a separation of public and private definitions into what should only be public definitions in the public header files. A couple of new header files placed alongside sources have been added for shared internal declarations and definitions (e.g., crown-internal.h). There should be no functional changes here.

I think most of what should be public and what should be private is pretty clear. There's some ambiguous stuff that I generally erred on the side of private with -- if it wasn't used outside of hatrack and it seemed not obviously public, I made it private. The rationale for this was that it's easier to add to the public API than it is to remove from it.

Most all internal struct definitions remain public, which I am personally not a fan of, particularly in something that's intended to be low-level and general purpose like this, but it seems this was intentional given the _init APIs and their use in con4m. I generally prefer to make public only what needs to be public as it eases the maintenance burden and makes intrusive changes that really shouldn't impact public use easier. There are ways to make it all work without exposing the data structures, but I haven't done that here and currently have no intention to do so.

There's some churn here with header files and also some formatting churn. There's a lot of clang-format off and clang-format on to preserve tabular formatting of function prototypes and the like. There were typos in some places that failed to re-enable formatting and sometimes re-enabling was just forgotten or whatever. Some of this kind of thing still remains in .c files, but I've cleaned it all up in the .h files.

The header file churn is largely due to the internal code not using the hatrack.h umbrella include, but in some cases also because of changes mainly to mmm.h that removed common includes like malloc.h and hatomic.h. This is a move closer to being IWYU (include what you use) clean. I didn't do a check with the tool to see what issues remain for this PR, but I will at some point soon to clean up the rest.

I've added HATRACK_EXTERN for function prototypes. It defaults to just extern, which of course isn't strictly necessary and wasn't used previously, at least for the most part. The intent is more for the future for other projects that embed. They can define HATRACK_EXTERN themselves if they want to. Mostly it's because I will want to when I embed this elsewhere to adjust symbol visibility. This should only be used for public functions. Don't use it for private functions shared between translation units.

A lot of static inline functions have been moved from being in headers, partly because mostly these are private functions, but also in a lot of cases because there's no justification for them being inline, particularly in mmm.h. The compiler won't actually inline all of them, many of them are doing heavy lifting that inlining won't help, and moving them to C translation units makes it easier to make changes, both planned and in a more general sense. If users of this library are dynamically linking it, having a lot of static inline functions for more than the simplest of things is not ideal.

Notably I've left debug.h and gate.h mostly untouched for now. I've taken them out of hatrack.h, though, at least for now. They both are not well name-spaced and have very specific use cases. The names in debug.h in particular are likely to clash with other code. Other than the gate code being used in the testing code, which already explicitly includes it, taking these two out of hatrack.h broke nothing. We can add them back into hatrack.h later if we really want to after they've been fixed, but fixing them was more than I wanted to take on here.

viega commented 3 months ago

I don't mind moving the data structures out of the public headers. The _init API was an easy way to avoid spending the time on a pluggable memory management API, but it was not meant to be permanent anyway, since we need dynamic allocation ourselves, and don't have access to their allocator.

orangematt commented 3 months ago

I don't mind moving the data structures out of the public headers. The _init API was an easy way to avoid spending the time on a pluggable memory management API, but it was not meant to be permanent anyway, since we need dynamic allocation ourselves, and don't have access to their allocator.

Ok. I can revisit that later. I think some of the con4m code is using the _init API, so it'd be a more widespread change, which I'm trying to avoid right now so I stay out of your way while you finish porting other bits over from NIm.