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

Improve deprecation of old enums #4944

Merged
merged 2 commits into from
Jan 20, 2019
Merged

Improve deprecation of old enums #4944

merged 2 commits into from
Jan 20, 2019

Conversation

ethomson
Copy link
Member

Add __attribute__((used)) to the GIT_DEPRECATED macro, otherwise this will be seen as an "unused const variable".

Also, use the enum type in the declaration of deprecated values. The C standard does not specify whether an enum is a signed or unsigned type. Obviously, any enum that includes negative values must be signed, but if all values are positive then the compiler is free to choose signed or unsigned.
Thus, we declare the deprecated values as the enum instead of guessing.

Recent GCC enables `-Wunused-const-variables`, which makes output quite
noisy.  Disable unused warnings for our deprecated variables.
The C standard does not specify whether an enum is a signed or unsigned
type.  Obviously, any enum that includes negative values _must_ be
signed, but if all values are positive then the compiler is free to
choose signed or unsigned.

Thus, by changing the type signatures to `git_object_t` and declaring
the old `GIT_OBJ_` values as a signed or unsigned int, we risk a
mismatch between what the compiler has chosen for a `git_object_t`'s
type and our type declaration.

Thus, we declare the deprecated values as the enum instead of guessing.
@ethomson
Copy link
Member Author

Note that I'm still a little bit on the fence about how we're deprecating these; I think that it might make sense to just keep them as #defines or what not until 1.0 ships, then mark them as deprecated. I don't think we need to be in any hurry to get rid of them. But I want more input on that.

@ethomson ethomson merged commit b5a3ef3 into master Jan 20, 2019
@ethomson ethomson deleted the ethomson/deprecation branch January 20, 2019 18:14
@pks-t
Copy link
Member

pks-t commented Jan 21, 2019

Yeah, I think keeping them as #defines instead of using static variables would be preferable. No increase in binary size, no problems with symbols being unused and it looks cleaner. Is it possible to deprecate macros though?

I don't think it's going to matter when we deprecate things: at some point in time, users will most likely be annoyed by the warnings. I'm rather pessimistic that anybody will actually update his code as long as they are not explicitly being marked as deprecated to emit warnings. Not saying this is due to lazyness, but more due to discoverability. I wouldn't think that every downstream user reads our release notes.

@ethomson
Copy link
Member Author

Is it possible to deprecate macros though?

AFAICT, no, it's not. Certainly not on all compilers, but maybe you could do some epic hackery to do something.

I'm rather pessimistic that anybody will actually update his code as long as they are not explicitly being marked as deprecated to emit warnings.

I agree. My counterpoint, though, is, do we ever really need to remove these for anything other than our own clutter? We could create deprecated.h, dump all these things in there, and forget about them forever.

This has the advantage of us being able to use #defines instead of static const int, keeping the size down and simplicity.

@ethomson
Copy link
Member Author

What I mentioned up there (the #define for now, mark them deprecated after 1.0) is a middle of the road approach that I'm not sure I really care about.

@pks-t
Copy link
Member

pks-t commented Jan 21, 2019

I'm fine with a deprecated.h header. As you said in another thread, we should include it by default in "libgit2.h" as long as nothing like e.g. LIBGIT2_NO_DEPRECATED is defined.

@tiennou
Copy link
Contributor

tiennou commented Jan 21, 2019

Are static const int counted as binary ? I was under the impression that, given const, they would be optimized to their integer representation anyway, but I could be wrong. This, plus the missing support for macros (I think clang was the culprit at the time) is why I favored not using a #define.

My POV is that, it depends on the amount of fixing needed by the deprecation. Things like git_buf_free/dispose are pretty mechanical changes, and I'd feel better to have them removed before 1.0.

@ethomson
Copy link
Member Author

My question is: why remove them at all?

@pks-t
Copy link
Member

pks-t commented Jan 22, 2019 via email

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.

None yet

4 participants