Skip to content
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

Set of exposed internal API is very wide. Impossible to guarantee stable API long term. #3568

Open
hzeller opened this issue Apr 6, 2023 · 1 comment

Comments

@hzeller
Copy link
Collaborator

hzeller commented Apr 6, 2023

On make install, a very large amount of headers is exported. This makes it impossible for internal refactorings to ever be possible as we've spilled all the implementation details externally, and Hyrum's law will make it impossible to change.

We have a single header that should specify the API for Surelog: Surelog/API/Surelog.h. However, now there are many headers that with there mere existence in an installation all give the impression to provide a stable API. We should strive to provide any user-visible stuff only in there or have a very limited set of includes that we actually export and are comfortable to keep stable long-term.

There is a kitchen-sink include that is exported (Surelog/surelog.h) which should only include the limited set of files we need to export (and eventually, we should eliminate this header as it is hard to reason what the users will actually use. IWYU!).

I suggest the following steps

  1. figure out what actually needs to be visible (I suspect something like error reporting, symbolId, PathId, symbol table, command line parser, maybe a few more)
  2. Look through these headers and make sure they don't accidentally rely on other things. Possibly cleanup.
  3. Add a #warning in the surelog. h header to encourage people to include what they need (IWYU).
  4. In a first step, reduce the amount of things included in surelog.h as determined in 1 and possibly user feedback.
  5. reduce the number of header exported in an make install to the relevant ones and pledge to keep them backward compatible long-term
hzeller added a commit to hzeller/Surelog that referenced this issue Apr 6, 2023
It makes it hard to reason which actual dependencies
are used. Being specific here allows us to narrow
down the APIs that we actually would like to make
public and provide long-term.

Issues: chipsalliance#3568

Signed-off-by: Henner Zeller <[email protected]>
@timkpaine
Copy link
Collaborator

its crazy the number of separate install commands we do for header files in the cmake file as well.

mandrys pushed a commit to antmicro/yosys-f4pga-plugins that referenced this issue May 31, 2023
The kitchen-sink include file Surelog/surelog.h makes things
look simple, but it also hides what we actually want.
So apply IWYU principle and only include what we need.

This will help us long-term to tackle
chipsalliance/Surelog#3568

Signed-off-by: Henner Zeller <[email protected]>
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

2 participants