Skip to content

Unused function warnings#4895

Merged
pks-t merged 12 commits intolibgit2:masterfrom
pks-t:pks/unused-warnings
Nov 29, 2018
Merged

Unused function warnings#4895
pks-t merged 12 commits intolibgit2:masterfrom
pks-t:pks/unused-warnings

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Nov 21, 2018

Commit 4b84db6 (patch_parse: remove unused function parse_number, 2018-11-14) led me to wonder why the compiler didn't emit any warnings at all for this unused function. Turns out we were disabling unused function warnings altogether back in 2014, which I think is unfortunate. Removing code is the best thing one can do if it doesn't break any features, so we should be notified by the compiler as soon as there's anything that's unneeded.

This PR re-enables the warnings. As it uses ENABLE_WARNING, unused functions will create compilation errors with -DENABLE_WERROR. Let's see how many builds break by this.

@ethomson
Copy link
Copy Markdown
Member

Neat. I'm 👍 on more better warnings as long as we don't have to jump through too many hoops (or #ifdef magic). Certainly looks like that's the case here. While we're doing this, curious what the impact of ENABLE_WARNINGS(unused-const-variable) is?

@pks-t pks-t force-pushed the pks/unused-warnings branch from 5ac0f69 to 4a26f14 Compare November 21, 2018 11:10
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 21, 2018

Yeah, it seems like we're mostly good and don't need any additional ifdef magic. The only ifdef change was in the win32 test stuff, but that one already used ifdefs, so we're no worse off than before.

I've noticed that there's also another unused function tree_iterator_entry_cmp, which you've added in be30387 (iterators: refactored tree iterator, 2016-02-25), but in fact it was dead from the beginning. Is this a bug or just a leftover?

And I didn't yet dare to touch unused-const-variable. I'd bet there are quite some headers that contain constant variables like that that aren't touched at all. But I'll check.

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 21, 2018

Okay, I think I know now what the issue was with those unused functions. It's mostly about khash, which we define as static inline functions in a header file. If that header is included but not all of its functions are used, then Clang will complain. I'll see if I can do something about it.

@neithernut
Copy link
Copy Markdown
Contributor

neithernut commented Nov 21, 2018

It appears that clang's -Wunneeded-internal-declaration is the closest to gcc's -Wunused-function (as found here) regarding the handling of static inline functions. But it may not be exactly equivalent. For example, it also emits warnings concerning variables. (clang also features a -Wunused-function option, which apparently doesn't take into account whether a function is inline or not).

Unfortunately, clang's documentation is rather lacking in a lot of places, including its command line options. In comparison, the the gcc man page is quite explicit.

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 28, 2018

/rebuild

@libgit2-azure-pipelines
Copy link
Copy Markdown

Okay, @pks-t, I started to rebuild this pull request.

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 28, 2018

@ethomson: got any idea why I can't see the build status here? Is it because I've force-pushed?

@ethomson
Copy link
Copy Markdown
Member

I'm not sure why it's not showing the "checks" section, but it is showing the red X next to commit 4a26f14. I'll see if I can understand what's going on here.

@ethomson
Copy link
Copy Markdown
Member

To expand on that comment: you can click on the red X to get to the build failure, but the checks section should still be shown. Not sure why it's not - I force push regularly and it doesn't disappear. 🙁

pks-t added 12 commits November 28, 2018 15:22
Currently, the "apply_helper" functions used for testing the apply logic are all
statically defined in the "apply_helpers.h" header file. This may lead to
warnings from the compiler in case where this header file is included, but not
all functions it brings along are used in the compilation unit where it has been
included into.

Fix these potential warnings by moving the implementation into its own
compilation unit "apply_helpers.c".
The function `test_canonicalize` is only used on Win32 platforms. It will thus
result in an unused function warning if these warnings are enabled and one is on
a platform different than Win32.

Fix the issue by only compiling in the function on Win32 platforms.
The function `tree_iterator_entry_cmp` has been introduced in commit be30387
(iterators: refactored tree iterator, 2016-02-25), but in fact it has never been
used at all. Remove it to avoid unused function warnings as soon as we re-enable
"-Wunused-functions".
Ever since commit 823c0e9 (Fix broken logic for attr cache invalidation,
2014-04-17), we have completely disabled warnings for unused functions. The only
comment that was added back then is about "annoying extra warnings" from Clang,
but in fact we shouldn't just ignore warnings about functions which aren't used
at all. Instead, the right thing would be to either only conditionally compile
functions that aren't used in all configurations or, alternatively, to remove
functions that aren't required at all.

As remaining instances of unused functions have been removed in the last two
commits, re-enable the warning.
The foreach macros of the idxmap types are not used anywhere. As we are
about to open-code all foreach macros for the maps in order to be able
to make the khash structure internal, removing these unused macros will
leave a few places less that need conversion.
The submodule code currently has its own implementation of a string map,
which overrides the hashing and hash equals functions with functions
that ignore potential trailing slashes. These functions aren't actually
used by our code, making them useless.
Right now, the `git_*map_begin()` and `git_*map_end()` helpers are
implemented via macros which simply redirect to `kh_begin` and `kh_end`.
As these macros refer to members of the map structures, they make it
impossible to move the khash include into the implementation files.

Implement these helpers as real functions instead to further decouple
the headers from implementations.
The current foreach map macros simply redirect to the type-indifferent
`kh_foreach` macro. As this type-indifferent macro directly accesses the
structures, the current implementation makes it impossible to make the
stuctures private to the implementation only. And making them private is
required to move out the khash include into the implementations to
decrease the namespace leak.
Instead of using the `khiter_t`, `git_strmap_iter` and `khint_t` types,
simply use `size_t` instead. This decouples code from the khash stuff
and makes it possible to move the khash includes into the implementation
files.
The current map implementations directly include the "khash.h" headers
into their own headers to make available a set of static functions,
defines et cetera. Besides leaking the complete khash namespace into
files wherever khashes are used, this also triggers Clang's
-Wunused-function warnings when some of the static functions are not
being used at all.

Fix the issue by moving the includes into the respective map
implementation files. Add forward declares for all the map types to make
them known.
The mailmap testdata header contains a set of static variable
definitions. As these variables aren't used in all places where they are
used, they trigger the unused-const-variable warnings. As we have
currently disabled those warnings explicitly, they are never triggered,
but we intend to enable them.

Avoid the issue by only keeping variable definitions that are actually
used in all locations. Move the others to where they are used.
Together with the warnings for unused warnings, we always had warnings
for unused constant variables disabled since commit 823c0e9 (Fix
broken logic for attr cache invalidation, 2014-04-17). As we have now
fixed all occurrences of such variables, we can safely enable those
warnings again.
@pks-t pks-t force-pushed the pks/unused-warnings branch from aff9d2d to ffe39ba Compare November 28, 2018 14:22
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 28, 2018

Thanks for your remarks by the way, @neithernut. I hope to be able to also get Clang to not complain about our inlines by limiting the khash scope, but your proposed flag would definitely be the fallback option in case I cannot get it to build.

@ethomson
Copy link
Copy Markdown
Member

Neat - looks like clang is happy indeed. Are you happy with this now @pks-t or were you thinking about changing something else?

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 29, 2018

No, I'm very happy with this PR. Finally we've contained all this khash stuff into where they are used to implement our map interfaces, which is a very good thing. Next, we should probably try to fix how the map interfaces look like -- they are still very macro-y, even though we are able to use function semantics just fine now.

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