Introduce GIT_WARN_UNUSED_RESULT #5802

lhchavez · 2021-02-17T03:40:13Z

This change adds the GIT_WARN_UNUSED_RESULT annotation, which makes the
compiler warn when a return result is not used. This avoids bugs.

ethomson · 2021-02-23T12:30:52Z

Naive question - is there an attribute for the inverse of this - where we accept that it's okay that the return value is unused?

I think that for most of our codebase, we should enforce checking return codes. I can only think of a few outliers (like the buf functions, where we accept that an OOM situation will eventually get checked, but we might chain several functions together before we do that check).

I have no idea how bad -Wunused-result would actually be, and it looks like it has some pain points of its own, I'm just curious.

lhchavez · 2021-02-24T01:39:58Z

Naive question - is there an attribute for the inverse of this - where we accept that it's okay that the return value is unused?

not as a standard attribute :( the closest thing would be a custom clang-tidy check. i personally wouldn't be opposed to investigating how to get one working, but it's probably better to start with an allow-list of functions we know we want to keep using their return values always plus possibly another macro to mark the ignoring of a return value as acceptable (i.e. this change), and once the whole codebase is clean, we can switch it around.

I think that for most of our codebase, we should enforce checking return codes. I can only think of a few outliers (like the buf functions, where we accept that an OOM situation will eventually get checked, but we might chain several functions together before we do that check).

I have no idea how bad -Wunused-result would actually be, and it looks like it has some pain points of its own, I'm just curious.

we're already using -Wunused-result! that's what powers the warnings for functions annotated with GIT_WARN_UNUSED: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says:

-Wno-unused-result
Do not warn if a caller of a function marked with attribute warn_unused_result (see Function Attributes) does not use its return value. The default is -Wunused-result.

lhchavez · 2021-02-24T03:37:00Z

Naive question - is there an attribute for the inverse of this - where we accept that it's okay that the return value is unused?

not as a standard attribute :( the closest thing would be a custom clang-tidy check. i personally wouldn't be opposed to investigating how to get one working, but it's probably better to start with an allow-list of functions we know we want to keep using their return values always plus possibly another macro to mark the ignoring of a return value as acceptable (i.e. this change), and once the whole codebase is clean, we can switch it around.

I think that for most of our codebase, we should enforce checking return codes. I can only think of a few outliers (like the buf functions, where we accept that an OOM situation will eventually get checked, but we might chain several functions together before we do that check).
I have no idea how bad -Wunused-result would actually be, and it looks like it has some pain points of its own, I'm just curious.

btw tweaking GIT_UNUSED() to 4ebb573#diff-07a0ff24f834c533d980cd176412c1cf4777562a3a10fe49ae049e06adff4487R46 eliminates most of the pain points. it uses a gcc-ism, but then again the GIT_WARN_UNUSED macro also does.

we're already using -Wunused-result! that's what powers the warnings for functions annotated with GIT_WARN_UNUSED: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says:

-Wno-unused-result
Do not warn if a caller of a function marked with attribute warn_unused_result (see Function Attributes) does not use its return value. The default is -Wunused-result.

ethomson · 2021-02-28T11:00:49Z

we're already using -Wunused-result! that's what powers the warnings for functions annotated with GIT_WARN_UNUSED: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says:

Ohhhh, I didn't fully understand the interaction here. This makes sense. Thanks for the explanation.

ethomson · 2021-03-04T10:15:50Z

src/sortedcache.h


 /* Unlock sorted cache when done with read */
 void git_sortedcache_runlock(git_sortedcache *sc);

 /* Lookup item by key - returns NULL if not found */
-void *git_sortedcache_lookup(const git_sortedcache *sc, const char *key);
+GIT_WARN_UNUSED_RESULT void *git_sortedcache_lookup(


I like GIT_WARN_UNUSED_RESULT for things that return errors that really must be paid attention to, for example things that allocate. I'm less bullish on things like this. One on hand, I think that I understand the motivation: why are you calling this function if you don't care about the result? It's side effect free. So its only purpose in life is to compute a result for you. Calling it and ignoring the result is just a waste of cycles.

🤔

I feel like we should draw up some simple guidelines about where and when we use GIT_WARN_UNUSED_RESULT... It feels like you're proposing a place where we land on it almost always. Is that roughly accurate?

i could also be okay with restricting its use to only places that allocate or grab a mutex (that case is also pretty scary).

if/when another clang-tidy solution exists where everything is checked except for a few allow-listed functions, things'll be less verbose and we'll get better coverage.

ethomson · 2021-07-30T13:20:59Z

Sorry for the long delay. One last question: does this need to be in the public api? I’d rather keep this defined in some header in src/

lhchavez · 2021-07-30T13:26:42Z

Sorry for the long delay. One last question: does this need to be in the public api? I’d rather keep this defined in some header in src/

it doesn't need to, let me move it!

lhchavez · 2021-07-30T13:44:04Z

Sorry for the long delay. One last question: does this need to be in the public api? I’d rather keep this defined in some header in src/

it doesn't need to, let me move it!

welp, not sure what happened, but now the MSVC builds are not picking the macro up :/

This change adds the GIT_WARN_UNUSED_RESULT annotation, which makes the compiler warn when a return result is not used. This avoids bugs.

This adds a `-Wunused-result`-proof `GIT_UNUSED()`, just to demonstrate that it works. With this, sortedcache.h is now completely `GIT_WARN_UNUSED_RESULT`-annotated!

Now we're limiting ourselves to only functions that allocate or acquire locks.

Previously, the location of `GIT_WARN_UNUSED_RESULT` was causing it to be included _after_ a bunch of other headers (namely `src/vector.h`), which broke the build. This change does two things: * Moves the `GIT_WARN_UNUSED_RESULT` above most of the `$include`s in `src/common.h`. * Stops including `vector.h` from `src/win32/path_w32.c` since the header itself does not use it.

ethomson · 2021-08-25T16:30:02Z

This is really nice. Thanks.

lhchavez force-pushed the git-warn-unused-result branch 2 times, most recently from 33ff48d to 67998a8 Compare February 23, 2021 01:11

lhchavez force-pushed the git-warn-unused-result branch from 4ebb573 to d7f0b72 Compare February 24, 2021 04:05

ethomson reviewed Mar 4, 2021

View reviewed changes

lhchavez force-pushed the git-warn-unused-result branch from f6ff300 to 68b7d2e Compare July 30, 2021 13:34

lhchavez force-pushed the git-warn-unused-result branch 2 times, most recently from 999262b to d63ce35 Compare July 30, 2021 14:32

lhchavez added 8 commits August 8, 2021 19:08

Introduce GIT_WARN_UNUSED_RESULT

9eb17d4

This change adds the GIT_WARN_UNUSED_RESULT annotation, which makes the compiler warn when a return result is not used. This avoids bugs.

Proof-of-concept for a more aggressive GIT_UNUSED()

231ca4f

This adds a `-Wunused-result`-proof `GIT_UNUSED()`, just to demonstrate that it works. With this, sortedcache.h is now completely `GIT_WARN_UNUSED_RESULT`-annotated!

Be a little bit less aggressive GIT_WARN_UNUSED_RESULT

4bb1568

Now we're limiting ourselves to only functions that allocate or acquire locks.

Add guidelines for the GIT_WARN_UNUSED_RESULT annotation

7488705

Handle one more unused variable introduced since the PR started

c4cbab3

Move GIT_WARN_UNUSED_RESULT from the public to the private API

4e8376a

formatting

991ccdc

lhchavez force-pushed the git-warn-unused-result branch 2 times, most recently from bd08c29 to 991ccdc Compare August 9, 2021 02:24

ethomson merged commit efc4e7e into libgit2:main Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce GIT_WARN_UNUSED_RESULT #5802

Introduce GIT_WARN_UNUSED_RESULT #5802

lhchavez commented Feb 17, 2021 •

edited

Loading

ethomson commented Feb 23, 2021

lhchavez commented Feb 24, 2021

lhchavez commented Feb 24, 2021

ethomson commented Feb 28, 2021

ethomson Mar 4, 2021

lhchavez Mar 4, 2021 •

edited

Loading

ethomson commented Jul 30, 2021

lhchavez commented Jul 30, 2021

lhchavez commented Jul 30, 2021

ethomson commented Aug 25, 2021

Introduce GIT_WARN_UNUSED_RESULT #5802

Introduce GIT_WARN_UNUSED_RESULT #5802

Conversation

lhchavez commented Feb 17, 2021 • edited Loading

ethomson commented Feb 23, 2021

lhchavez commented Feb 24, 2021

lhchavez commented Feb 24, 2021

ethomson commented Feb 28, 2021

ethomson Mar 4, 2021

Choose a reason for hiding this comment

lhchavez Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

ethomson commented Jul 30, 2021

lhchavez commented Jul 30, 2021

lhchavez commented Jul 30, 2021

ethomson commented Aug 25, 2021

lhchavez commented Feb 17, 2021 •

edited

Loading

lhchavez Mar 4, 2021 •

edited

Loading