Silence build-time warning coming from scope.h

[jkeenan]

In our ticket tracking build-time warnings generated during make when using clang (especially versions 9, 10 and 11) to build perl, #17015, the last remaining such warning looked like this:

{
    char   => 37,
    group  => "Wstring-compare",
    line   => 8219,
    source => "re_comp.c",
    text   => "result of comparison against a string literal is unspecified (use an explicit string comparison function instead)",
  },
  {
    char   => 2,
    group  => "Wstring-compare",
    line   => 8267,
    source => "re_comp.c",
    text   => "result of comparison against a string literal is unspecified (use an explicit string comparison function instead)",
  },

Analysis of the make output showed that the source of the problem lay in scope.h. This pull request simplifies some code in that file and silences that build-time warning.

(Unfortunately, it does not mean we've achieved warnings-free building with clang9 or clang10. Since that time a build-time warning has crept into cpan/Scalar-List-Utils/ListUtil.xs, for which I have filed a bug ticket upstream.)

(Also, we still have to face the problem of many thousands of build-time warnings being generated when we configure with clang12.)

Thank you very much.
Jim Keenan

[tonycoz]

Your change effectively reverts d0f83c3.

The identity check:

(char*)PL_scopestack_name[PL_scopestack_ix-1] == (char*)name

is intended to shortcut the comparison and I believe our use here is valid, since it's combined with the string comparison.

But I think the warning is in general a good warning.

Instead of removing the comparison (or suppressing the warning globally) we could suppress the warning for this case:

#define LEAVE_with_name(name)						\
    STMT_START {							\
        DEBUG_SCOPE("LEAVE \"" name "\"")				\
        if (PL_scopestack_name)	{					\
            CLANG_DIAG_IGNORE_STMT(-Wstring-compare);			\
            assert(((char*)PL_scopestack_name[PL_scopestack_ix-1]	\
                        == (char*)name)					\
                    || strEQ(PL_scopestack_name[PL_scopestack_ix-1], name));        \
            CLANG_DIAG_RESTORE_STMT;					\
        }								\
        pop_scope();							\
    } STMT_END

[jkeenan]

Your change effectively reverts d0f83c3.

The identity check:

(char*)PL_scopestack_name[PL_scopestack_ix-1] == (char*)name

is intended to shortcut the comparison and I believe our use here is valid, since it's combined with the string comparison.

But I think the warning is in general a good warning.

Instead of removing the comparison (or suppressing the warning globally) we could suppress the warning for this case:

#define LEAVE_with_name(name)						\
    STMT_START {							\
        DEBUG_SCOPE("LEAVE \"" name "\"")				\
        if (PL_scopestack_name)	{					\
            CLANG_DIAG_IGNORE_STMT(-Wstring-compare);			\
            assert(((char*)PL_scopestack_name[PL_scopestack_ix-1]	\
                        == (char*)name)					\
                    || strEQ(PL_scopestack_name[PL_scopestack_ix-1], name));        \
            CLANG_DIAG_RESTORE_STMT;					\
        }								\
        pop_scope();							\
    } STMT_END

Thanks for the review. I tested it with each of gcc and g++ as C-compiler and the warning was suppressed. Can you explain how the use of a macro with 'CLANG' in its name covers those two cases as well?

Branch updated.

[tonycoz]

Can you explain how the use of a macro with 'CLANG' in its name covers those two cases as well?

It's not documented, but I suspect gcc recognizes the pragma that macro generates - clang is documented to recognize the GCC version of the pragma.

If both compilers are warning on this code then using the gcc version might be better, since clang is documented to recognize the gcc pragma.

[jkeenan]

Can you explain how the use of a macro with 'CLANG' in its name covers those two cases as well?

It's not documented, but I suspect gcc recognizes the pragma that macro generates - clang is documented to recognize the GCC version of the pragma.

If both compilers are warning on this code then using the gcc version might be better, since clang is documented to recognize the gcc pragma.

I updated the branch/pull request as follows:

$ git show
commit 8c417c6768b722f32b344fa61c0adecd243abccd (HEAD -> smoke-me/jkeenan/scope-h-build-time-warning-20210625, origin/smoke-me/jkeenan/scope-h-build-time-warning-20210625)
Author: James E Keenan <jkeenan@cpan.org>
Date:   Wed Jun 30 01:35:20 2021 +0000

    Per TonyC, use GCC variant of macros

diff --git a/scope.h b/scope.h
index f38e7edb55..ba74c8bff5 100644
--- a/scope.h
+++ b/scope.h
@@ -208,11 +208,11 @@ scope has the given name. C<name> must be a literal string.
     STMT_START {                                                       \
         DEBUG_SCOPE("LEAVE \"" name "\"")                              \
         if (PL_scopestack_name)        {                                       \
-            CLANG_DIAG_IGNORE_STMT(-Wstring-compare);                  \
+            GCC_DIAG_IGNORE_STMT(-Wstring-compare);                    \
             assert(((char*)PL_scopestack_name[PL_scopestack_ix-1]      \
                         == (char*)name)                                        \
                     || strEQ(PL_scopestack_name[PL_scopestack_ix-1], name));        \
-            CLANG_DIAG_RESTORE_STMT;                                   \
+            GCC_DIAG_RESTORE_STMT;                                     \
         }                                                              \
         pop_scope();                                                   \
     } STMT_END

I then built with each of the 3 compilers in turn. In each case, the warnings coming from scope.h were not generated.

Please review.

[tonycoz]

The final result looks reasonable, but please squash and force push the PR branch.