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

fileio_types.h : avoid dependency on mem.h #3232

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Aug 3, 2022

fileio_types.h cannot be parsed by itself
because it relies on basic types defined in lib/common/mem.h.
As for #3231, it likely wasn't detected because mem.h was probably included before fileio_types.h in target files.

An "easy" solution would be to just add the missing include,
but each #include dependency must be considered "bad" by default,
and only allowed if it brings some tangible value.

In this case, since these types are only used to declare internal structure variables
which are effectively only flags,
I believe it's not valuable to add a dependency on mem.h for this purpose
while the standard int type can do the same job.

I was expecting some compiler warnings following this change,
but it turns out we don't use -Wconversion by default on zstd source code,
so there is none.

Nevertheless, I enabled -Wconversion locally and proceeded to fix a few conversion warnings in the process.

Sidenote:
Adding -Wconversion to the list of flags used for zstd is something I would be favorable over the long term,
but it cannot be done overnight,
because the nb of places where this warning is triggered is daunting.
Better progressively reduce the nb of triggered -Wconversion warnings before enabling this flag by default.

fileio_types.h cannot be parsed by itself
because it relies on basic types defined in `lib/common/mem.h`.
As for #3231, it likely wasn't detected because `mem.h` was probably included before within target files.
But this is not proper.

A "easy" solution would be to add the missing include,
but each dependency should be considered "bad" by default,
and only allowed if it brings some tangible value.

In this case, since these types are only used to declare internal structure variables
which are effectively only flags,
I believe it's really not valuable to add a dependency on `mem.h` for this purpose
while the standard `int` type can do the same job.

I was expecting some compiler warnings following this change,
but it turns out we don't use `-Wconversion` by default on `zstd` source code,
so there is none.

Nevertheless, I enabled `-Wconversion` locally and proceeded to fix a few conversion warnings in the process.

Adding `-Wconversion` to the list of flags used for `zstd` is something I would be favorable over the long term,
but it cannot be done overnight,
because the nb of places where this warning is triggered is daunting.
Better progressively reduce the nb of triggered `-Wconversion` warnings before enabling this flag by default.
@Cyan4973 Cyan4973 merged commit e818fa8 into dev Aug 3, 2022
@Cyan4973 Cyan4973 deleted the fileiotypes_nomemh branch January 13, 2023 04:28
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants