Bug 197148 - [aarch64] clang Wc++11-narrowing for unsigned char in WebCore/contentextensions
Summary: [aarch64] clang Wc++11-narrowing for unsigned char in WebCore/contentextensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Other
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-21 08:08 PDT by wk@unrelenting.technology
Modified: 2019-04-24 08:52 PDT (History)
5 users (show)

See Also:


Attachments
webkit-schar.patch (1.23 KB, patch)
2019-04-21 08:08 PDT, wk@unrelenting.technology
no flags Details | Formatted Diff | Diff
webkit-schar.patch v2 (2.07 KB, patch)
2019-04-23 08:51 PDT, wk@unrelenting.technology
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wk@unrelenting.technology 2019-04-21 08:08:56 PDT
Created attachment 367910 [details]
webkit-schar.patch

Building WebKitGtk+ 2.24.0 on FreeBSD/aarch64 (13-CURRENT) results in signed char warnings (that get turned into errors by Werror):

http://thunderx1.nyi.freebsd.org/data/head-arm64-default/p498440_s346039/logs/errors/webkit2-gtk3-2.24.0_1.log

/wrkdirs/usr/ports/www/webkit2-gtk3/work/webkitgtk-2.24.0/Source/WebCore/contentextensions/DFACombiner.cpp:109:52: error: non-constant-expression cannot be narrowed from type 'char' to 'signed char' in initializer list [-Wc++11-narrowing]
                m_output.transitionRanges.append({ range.first, range.last });
                                                   ^~~~~~~~~~~
/wrkdirs/usr/ports/www/webkit2-gtk3/work/webkitgtk-2.24.0/Source/WebCore/contentextensions/DFACombiner.cpp:109:52: note: insert an explicit cast to silence this issue
                m_output.transitionRanges.append({ range.first, range.last });
                                                   ^~~~~~~~~~~
                                                   static_cast<signed char>( )
/wrkdirs/usr/ports/www/webkit2-gtk3/work/webkitgtk-2.24.0/Source/WebCore/contentextensions/DFACombiner.cpp:109:65: error: non-constant-expression cannot be narrowed from type 'char' to 'signed char' in initializer list [-Wc++11-narrowing]
                m_output.transitionRanges.append({ range.first, range.last });
                                                                ^~~~~~~~~~
/wrkdirs/usr/ports/www/webkit2-gtk3/work/webkitgtk-2.24.0/Source/WebCore/contentextensions/DFACombiner.cpp:109:65: note: insert an explicit cast to silence this issue
                m_output.transitionRanges.append({ range.first, range.last });
                                                                ^~~~~~~~~~
                                                                static_cast<signed char>( )

The attached patch fixes this by using signed chars in the types of the 'range' objects.
Comment 1 Alexey Proskuryakov 2019-04-22 16:46:53 PDT
Comment on attachment 367910 [details]
webkit-schar.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367910&action=review

> Source/WebCore/contentextensions/DFACombiner.cpp:40
> +    typedef MutableRangeList<signed char, uint64_t, 128> CombinedTransitionsMutableRangeList;

WebKit coding style is to use one word (signed/unsigned, not signed char/unsigned char).

I don't know if we necessarily want to fix this warning in code, it looks overly pedantic. But to have this discussed, it will be useful to formally post it for review (with a ChangeLog, and marked r?). Would you be willing to do that?
Comment 2 Alex Christensen 2019-04-22 16:52:07 PDT
Comment on attachment 367910 [details]
webkit-schar.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367910&action=review

>> Source/WebCore/contentextensions/DFACombiner.cpp:40
>> +    typedef MutableRangeList<signed char, uint64_t, 128> CombinedTransitionsMutableRangeList;
> 
> WebKit coding style is to use one word (signed/unsigned, not signed char/unsigned char).
> 
> I don't know if we necessarily want to fix this warning in code, it looks overly pedantic. But to have this discussed, it will be useful to formally post it for review (with a ChangeLog, and marked r?). Would you be willing to do that?

This is true with things like integers, but chars are a mess.  They're usually unsigned on linux and signed most other places but not all.  This needs the signed keyword to be portable.  If you request review and commit, I'll do both.
Comment 3 Alex Christensen 2019-04-22 16:53:03 PDT
You'll also need a ChangeLog entry.  Running Tools/Scripts/prepare-ChangeLog will give you a template to fill in.
Comment 4 Michael Catanzaro 2019-04-23 07:54:06 PDT
Yeah, I agree this change is correct. signed or unsigned on its own would be signed int or unsigned int, not what's needed here.

Remember signed char is a separate type from char (unlike all other signed types).
Comment 5 wk@unrelenting.technology 2019-04-23 08:51:57 PDT
Created attachment 368036 [details]
webkit-schar.patch v2

added changelog entry
Comment 6 Alex Christensen 2019-04-24 08:24:53 PDT
Comment on attachment 368036 [details]
webkit-schar.patch v2

See https://github.com/llvm/llvm-project/blob/master/lldb/source/Utility/ArchSpec.cpp#L763 for why this kind of thing is necessary :/
Comment 7 WebKit Commit Bot 2019-04-24 08:51:45 PDT
Comment on attachment 368036 [details]
webkit-schar.patch v2

Clearing flags on attachment: 368036

Committed r244591: <https://trac.webkit.org/changeset/244591>
Comment 8 WebKit Commit Bot 2019-04-24 08:51:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-04-24 08:52:19 PDT
<rdar://problem/50169088>