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

Make sure serializers include the config.h #1005

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

hzeller
Copy link
Collaborator

@hzeller hzeller commented Aug 28, 2023

The files should be self-contained and include the header that defines the windows-specific O_BINARY.
(Currently that only works accidentally if compiled with cmake)

The files should be self-contained and include the header that
defines the windows-specific O_BINARY.
(Currently that only works accidentally if compiled with cmake)
@hs-apotell
Copy link
Collaborator

I don't think this change is required. Config.h is included as a force include and shouldn't be included explicitly. Any build system, not just CMake, will need to do the same i.e. force include as command line arg to compiler.

@hzeller
Copy link
Collaborator Author

hzeller commented Aug 28, 2023

Compilation units should always be self-contained and include everything it needs, in particular when it is to be distributed - you can't control how people will compile a library, so force-includes on the command line is not really used for that.

@hs-apotell
Copy link
Collaborator

But, that's exactly how config.h is included in the project. None of the files include config.h explicitly. This is not limited to serializer only implementation. Same is true for Surelog project as well.

I agree that headers should be explicitly included but, in my opinion, configuration headers are out of that scope. On top that, config.h doesn't actually exist. It is generated by CMake, and so any build system will have to do something similar.

@hzeller
Copy link
Collaborator Author

hzeller commented Aug 28, 2023

But, that's exactly how config.h is included in the project.
None of the files include config.h explicitly. This is not limited to serializer only implementation.

Right, but only the serializers are using it currently, that is why I included it there. It really should be included in all places.

Same is true for Surelog project as well.

Yes, I noticed that, and I will fix that in a later PR there :)

I agree that headers should be explicitly included

Yes. Let's apply it always :)

but, in my opinion, configuration headers are out of that scope.

Including config headers is the only way to guarantee what we need. So it should also be considered in the same scope.
For the reason that you can't enforce any way for users of the library to use that particular way, this is needed.

On top that, config.h doesn't actually exist. It is generated by CMake, and so any build system will have to do something similar.

config.h is generated and will be distributed along with the other headers, so it will work well when included in files.
Requiring to include via the command line is somewhat orthogonal to that.

@alaindargelas alaindargelas merged commit 6aa3bd5 into chipsalliance:master Aug 29, 2023
6 checks passed
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

Successfully merging this pull request may close these issues.

3 participants