Conversation
|
Neat. I'm 👍 on more better warnings as long as we don't have to jump through too many hoops (or |
5ac0f69 to
4a26f14
Compare
|
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 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. |
|
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. |
|
It appears that clang's 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. |
|
/rebuild |
|
Okay, @pks-t, I started to rebuild this pull request. |
|
@ethomson: got any idea why I can't see the build status here? Is it because I've force-pushed? |
|
I'm not sure why it's not showing the "checks" section, but it is showing the red X next to commit |
|
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. 🙁 |
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.
aff9d2d to
ffe39ba
Compare
|
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. |
|
Neat - looks like clang is happy indeed. Are you happy with this now @pks-t or were you thinking about changing something else? |
|
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. |
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.