refs: forbid clang to complain about unreachable code
PATCH 1/1 refs: forbid clang to complain about unreachable code
From: Johannes Schindelin <[email protected]>
When NO_SYMLINK_HEAD is defined, create_ref_symlink() is hard-coded as (-1), and as a consequence the condition !create_ref_symlink() always evaluates to false, rendering any code guarded by that condition unreachable.
Therefore, clang is _technically_ correct when it complains about unreachable code. It does completely miss the fact that this is okay because on _other_ platforms, where NO_SYMLINK_HEAD is not defined, the code isn't unreachable at all.
Let's use the same trick as in 82e79c63642c (git-compat-util: add NOT_CONSTANT macro and use it in atfork_prepare(), 2025-03-17) to appease clang while at the same time keeping the -Wunreachable flag to potentially find _actually_ unreachable code.
---
refs: forbid clang to complain about unreachable code
Just upstreamin'
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1984%2Fdscho%2Frefs-clang-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1984/dscho/refs-clang-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1984
refs/files-backend.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
refs/files-backend.c +7 -1
@@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,* next update. If not, we try and create a regular symref. */ if (update->new_target && refs->prefer_symlink_refs) -if (!create_ref_symlink (lock, update->new_target)) +/* +* By using the `NOT_CONSTANT()` trick, we can avoid +* errors by `clang`'s `-Wunreachable` logic that would +* report that the `continue` statement is not reachable +* when `NO_SYMLINK_HEAD` is `#define`d. +*/ +if (NOT_CONSTANT (!create_ref_symlink (lock, update->new_target)))continue ;if (update->flags &REF_NEEDS_COMMIT ) {
base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 17 lines · writes:
From: Johannes Schindelin <[email protected]>
When
NO_SYMLINK_HEADis defined,create_ref_symlink()is hard-coded as(-1), and as a consequence the condition!create_ref_symlink()always evaluates to false, rendering any code guarded by that condition unreachable.Therefore, clang is _technically_ correct when it complains about unreachable code. It does completely miss the fact that this is okay because on _other_ platforms, where
NO_SYMLINK_HEADis not defined, the code isn't unreachable at all.Let's use the same trick as in
82e79c63642c(git-compat-util: addNOT_CONSTANTmacro and use it inatfork_prepare(), 2025-03-17) to appease clang while at the same time keeping the-Wunreachableflag to potentially find _actually_ unreachable code.
Quoted 5 lines
Signed-off-by Johannes Schindelin <[email protected]>---
refs: forbid clang to complain about unreachable codeJust upstreamin'
It may not be a bad idea to deprecate core.preferSymlinkRefs now and remove it at Git 3.0 boundary. Some platforms may not be able to do symbolic links and use it to represent HEAD, but everybody should be able to create a small text file with a single line.
But until then, this is a very reasonable thing to do.
Thanks.
Quoted 29 lines
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1984%2Fdscho%2Frefs-clang-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1984/dscho/refs-clang-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1984refs/files-backend.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)refs/files-backend.c +7
-1@@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,* next update. If not, we try and create a regular symref. */ if (update->new_target && refs->prefer_symlink_refs) -if (!create_ref_symlink (lock, update->new_target)) +/* +* By using the `NOT_CONSTANT()` trick, we can avoid +* errors by `clang`'s `-Wunreachable` logic that would +* report that the `continue` statement is not reachable +* when `NO_SYMLINK_HEAD` is `#define`d. +*/ +if (NOT_CONSTANT (!create_ref_symlink (lock, update->new_target)))continue ;if (update->flags &REF_NEEDS_COMMIT ) {base-commit:
c44beea485f0f2feaf460e2ac87fdd5608d63cf0
Quoted 24 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 21 lines · writes:
From: Johannes Schindelin <[email protected]>
When
NO_SYMLINK_HEADis defined,create_ref_symlink()is hard-coded as(-1), and as a consequence the condition!create_ref_symlink()always evaluates to false, rendering any code guarded by that condition unreachable.Therefore, clang is _technically_ correct when it complains about unreachable code. It does completely miss the fact that this is okay because on _other_ platforms, where
NO_SYMLINK_HEADis not defined, the code isn't unreachable at all.Let's use the same trick as in
82e79c63642c(git-compat-util: addNOT_CONSTANTmacro and use it inatfork_prepare(), 2025-03-17) to appease clang while at the same time keeping the-Wunreachableflag to potentially find _actually_ unreachable code.Signed-off-by Johannes Schindelin <[email protected]>---
refs: forbid clang to complain about unreachable code
Quoted 8 lines
Quoted 1 line
Just upstreamin'
It may not be a bad idea to deprecate core.preferSymlinkRefs now and remove it at Git 3.0 boundary. Some platforms may not be able to do symbolic links and use it to represent HEAD, but everybody should be able to create a small text file with a single line.
But until then, this is a very reasonable thing to do.
Agreed. I don't see any reason why anyone would like to use symbolic refs for this. The reading side for such symrefs may continue to exist for a while. But the writing side can go away.
Patrick
Quoted 7 lines · Johannes Schindelin via GitGitGadget
refs/files-backend.c +0
-0@@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,* next update. If not, we try and create a regular symref. */
refs/files-backend.c static int files_transaction_finish(struct ref_store *ref_store, · line 3196
Quoted 10 lines
refs/files-backend.c +7
-1if (update->new_target && refs->prefer_symlink_refs) -if (!create_ref_symlink (lock, update->new_target)) +/* +* By using the `NOT_CONSTANT()` trick, we can avoid +* errors by `clang`'s `-Wunreachable` logic that would +* report that the `continue` statement is not reachable +* when `NO_SYMLINK_HEAD` is `#define`d. +*/ +if (NOT_CONSTANT (!create_ref_symlink (lock, update->new_target)))continue ;
An alternative could be to fix this at the source, e.g. like the below (untested) patch. But I don't mind this too much, especially given that this here is the only callsite of that function anyway. So please feel free to disregard.
Thanks!
Patrick
refs/files-backend.c +1 -1
@@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs, }#ifdef NO_SYMLINK_HEAD -#define create_ref_symlink (a, b) (-1) +#define create_ref_symlink (a, b) NOT_CONSTANT(-1)#else static int create_ref_symlink (struct ref_lock *lock,const char *target) {
Hi Patrick,
refs/files-backend.c static int commit_ref_update(struct files_ref_store *refs, · line 2121
Quoted 13 lines · Patrick Steinhardt
refs/files-backend.c +1
-1@@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs, }#ifdef NO_SYMLINK_HEAD -#define create_ref_symlink (a, b) (-1) +#define create_ref_symlink (a, b) NOT_CONSTANT(-1)#else static int create_ref_symlink (struct ref_lock *lock,const char *target) {
While this is correct, and "closer to the root", in my experience it is better to have work-arounds closer to where the symptom appears. In this case, it would be directly in the condition of the if () construct. Therefore, I would prefer to keep the proposed patch as-is.
Ciao,
Johannes
Quoted 11 lines · Johannes Schindelin
Hi Patrick,
Quoted 7 lines · Patrick Steinhardt
refs/files-backend.c +0
-0@@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs, }
Quoted 11 lines
refs/files-backend.c static int commit_ref_update(struct files_ref_store *refs, · line 2121
Quoted 6 lines
refs/files-backend.c +1
-1#ifdef NO_SYMLINK_HEAD -#define create_ref_symlink (a, b) (-1) +#define create_ref_symlink (a, b) NOT_CONSTANT(-1)#else static int create_ref_symlink (struct ref_lock *lock,const char *target) {While this is correct, and "closer to the root", in my experience it is better to have work-arounds closer to where the symptom appears. In this case, it would be directly in the condition of the
if ()construct. Therefore, I would prefer to keep the proposed patch as-is.
As said, I don't feel strongly about it, so this is totally fine with me. Thanks!
Patrick
Quoted 6 lines · Patrick Steinhardt
An alternative could be to fix this at the source, e.g. like the below (untested) patch. But I don't mind this too much, especially given that this here is the only callsite of that function anyway. So please feel free to disregard.
Thanks!
Indeed that is much nicer as it targets only NO_SYMLINK_HEAD case without affecting the other side of #ifdef/#else/#endif but as you say, I think this falls into "once the code is written one way, it is not worth revisiting to change it with more iterations" category.
Thanks for very sharp eyes, nevertheless.
Quoted 13 lines
refs/files-backend.c +1
-1@@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs, }#ifdef NO_SYMLINK_HEAD -#define create_ref_symlink (a, b) (-1) +#define create_ref_symlink (a, b) NOT_CONSTANT(-1)#else static int create_ref_symlink (struct ref_lock *lock,const char *target) {