Bug 95468

Created attachment 161657 [details] Patch

Attachment 161657 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ChangeLog', u'Sour..." exit_code: 1 Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:712: More than one command on the same line in if [whitespace/parens] [4] Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:762: More than one command on the same line in if [whitespace/parens] [4] Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:771: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

The style errors existed in ANGLE codebase already, I didn't change them on purpose. Should I though?

(In reply to comment #3) > The style errors existed in ANGLE codebase already, I didn't change them on purpose. Should I though? > The previous code also had this problem(in correct style) i guess. The webkit style check script applies only to the code that has been changed. That is only to code added/modified by your patch. just bring the line content after the if() to the line below. See http://www.webkit.org/coding/coding-style.html Hope this helps...

This means though that the patch will break the code style of the surrounding file, just to get it accepted in WebKit? Assuming I upstream the patch as-is to ANGLE, would I still need to change it to get it into WebKit and thus create a diversion?

Comment on attachment 161657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161657&action=review > Source/ThirdParty/ANGLE/ChangeLog:12 > + Add the explicit std:: qualifier to uses of std functions and types. > + On other platforms this is apparently not required due to implicit > + inclusion of "string.h"-style headers containing extern "C" variants. > + On QNX at least this is not the case though, resulting in compile > + errors. Wouldn't a simpler way to fix this would be to add one or more includes of <string.h>?

also blocks 74040

Upstream patch: https://codereview.appspot.com/6653048

(In reply to comment #6) > (From update of attachment 161657 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161657&action=review > > > Source/ThirdParty/ANGLE/ChangeLog:12 > > + Add the explicit std:: qualifier to uses of std functions and types. > > + On other platforms this is apparently not required due to implicit > > + inclusion of "string.h"-style headers containing extern "C" variants. > > + On QNX at least this is not the case though, resulting in compile > > + errors. > > Wouldn't a simpler way to fix this would be to add one or more includes of <string.h>? Millian, did you see Darin's comment? If that works then it would seem like a much simpler solution :)

Yes, I saw Darin's comment, but wonder why it should be done instead of adding the explicit std:: qualified identifier. Example: When trying to compile DirectiveParser.cpp into DirectiveParser.o, we get (without this patch): qcc -Vgcc_ntoarmv7le -c -Wno-psabi -lang-c++ -O2 -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -D_REENTRANT -DQT_NO_CLIPBOARD -DQT_NO_LIBUDEV -DQT_NO_EVDEV -DQT_NO_XCB -DBUILDING_QT__=1 -DNDEBUG -DENABLE_3D_RENDERING=1 -DENABLE_BLOB=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CSS_BOX_DECORATION_BREAK=1 -DENABLE_CSS_COMPOSITING=1 -DENABLE_CSS_EXCLUSIONS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_CSS_IMAGE_SET=1 -DENABLE_CSS_REGIONS=1 -DENABLE_CSS_STICKY_POSITION=1 -DENABLE_DATALIST_ELEMENT=1 -DENABLE_DETAILS_ELEMENT=1 -DENABLE_FAST_MOBILE_SCROLLING=1 -DENABLE_FILTERS=1 -DENABLE_FTPDIR=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_ICONDATABASE=1 -DENABLE_IFRAME_SEAMLESS=1 -DENABLE_INPUT_TYPE_COLOR=1 -DENABLE_INSPECTOR=1 -DENABLE_INSPECTOR_SERVER=1 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_LEGACY_VIEWPORT_ADAPTION=1 -DENABLE_LEGACY_VENDOR_PREFIXES=1 -DENABLE_METER_ELEMENT=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_PROGRESS_ELEMENT=1 -DENABLE_RESOLUTION_MEDIA_QUERY=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SQL_DATABASE=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DWTF_USE_TILED_BACKING_STORE=1 -DHAVE_QTQUICK=1 -DHAVE_QTPRINTSUPPORT=1 -DHAVE_QSTYLE=1 -DHAVE_QTTESTLIB=1 -DWTF_USE_ZLIB=1 -DPLUGIN_ARCHITECTURE_UNSUPPORTED=1 -DWTF_USE_3D_GRAPHICS=1 -DENABLE_WEBGL=1 -DENABLE_CSS_SHADERS=1 -DENABLE_TOUCH_SLIDER=1 -DENABLE_ACCELERATED_2D_CANVAS=0 -DENABLE_ANIMATION_API=0 -DENABLE_BATTERY_STATUS=0 -DENABLE_CSP_NEXT=0 -DENABLE_CSS_GRID_LAYOUT=0 -DENABLE_CSS_HIERARCHIES=0 -DENABLE_CSS_IMAGE_ORIENTATION=0 -DENABLE_CSS_IMAGE_RESOLUTION=0 -DENABLE_CSS_VARIABLES=0 -DENABLE_CSS3_CONDITIONAL_RULES=0 -DENABLE_CSS3_TEXT=0 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATAGRID=0 -DENABLE_DATA_TRANSFER_ITEMS=0 -DENABLE_DEVICE_ORIENTATION=0 -DENABLE_DIRECTORY_UPLOAD=0 -DENABLE_DOWNLOAD_ATTRIBUTE=0 -DENABLE_FILE_SYSTEM=0 -DENABLE_GAMEPAD=0 -DENABLE_GEOLOCATION=0 -DENABLE_HIGH_DPI_CANVAS=0 -DENABLE_INDEXED_DATABASE=0 -DENABLE_INPUT_SPEECH=0 -DENABLE_INPUT_TYPE_DATE=0 -DENABLE_INPUT_TYPE_DATETIME=0 -DENABLE_INPUT_TYPE_DATETIMELOCAL=0 -DENABLE_INPUT_TYPE_MONTH=0 -DENABLE_INPUT_TYPE_TIME=0 -DENABLE_INPUT_TYPE_WEEK=0 -DENABLE_LEGACY_CSS_VENDOR_PREFIXES=0 -DENABLE_LINK_PREFETCH=0 -DENABLE_LINK_PRERENDER=0 -DENABLE_MATHML=0 -DENABLE_MEDIA_SOURCE=0 -DENABLE_MEDIA_STATISTICS=0 -DENABLE_MEDIA_STREAM=0 -DENABLE_MHTML=0 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=0 -DENABLE_NAVIGATOR_CONTENT_UTILS=0 -DENABLE_NETSCAPE_PLUGIN_API=0 -DENABLE_NETWORK_INFO=0 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_QUOTA=0 -DENABLE_SCRIPTED_SPEECH=0 -DENABLE_SHADOW_DOM=0 -DENABLE_STYLE_SCOPED=0 -DENABLE_SVG_DOM_OBJC_BINDINGS=0 -DENABLE_TEXT_AUTOSIZING=0 -DENABLE_TEXT_NOTIFICATIONS_ONLY=0 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_UNDO_MANAGER=0 -DENABLE_VIBRATION=0 -DENABLE_VIDEO=0 -DENABLE_VIDEO_TRACK=0 -DENABLE_WEB_AUDIO=0 -DENABLE_XSLT=0 -DBUILDING_ANGLE -DBUILDING_WEBKIT -DQT_ASCII_CAST_WARNINGS -DQT_NO_EXCEPTIONS -D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE -DQT_NO_DEBUG -I/home/milian/projects/qt5/install-qnx650/mkspecs/qnx-armv7le-qcc -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE -I.moc/debug-shared -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/include/GLSLANG -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new -I/home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/include -I/home/milian/projects/qt5/qtwebkit/Source -I/home/milian/projects/qt5/build-qnx650/qtwebkit/Source/include -I/home/milian/projects/qt5/install-qnx650/include -I/home/milian/projects/qt5/install-qnx650/include/QtScript -I/opt/qnx650/target/qnx6/usr/include -I/opt/qnx650/target/qnx6/usr/include/freetype2 -I. -o obj/debug/src/compiler/preprocessor/new/DirectiveParser.o /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp In file included from /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:16: /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:55: error: 'size_t' does not name a type /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In constructor 'pp::MacroExpander::MacroContext::MacroContext()': /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:58: error: class 'pp::MacroExpander::MacroContext' does not have any field named 'index' /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In member function 'bool pp::MacroExpander::MacroContext::empty() const': /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:59: error: ISO C++ forbids comparison between pointer and integer /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In member function 'const pp::Token& pp::MacroExpander::MacroContext::get()': /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:60: error: ISO C++ forbids incrementing a pointer of type 'const char* (*)(const char*, int)' /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:60: error: lvalue required as increment operand /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h: In member function 'void pp::MacroExpander::MacroContext::unget()': /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:61: error: ISO C++ forbids decrementing a pointer of type 'const char* (*)(const char*, int)' /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.h:61: error: lvalue required as decrement operand /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp: In member function 'void pp::DirectiveParser::parseVersion(pp::Token*)': /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:712: error: 'atoi' was not declared in this scope /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp: In member function 'void pp::DirectiveParser::parseLine(pp::Token*)': /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:762: error: 'atoi' was not declared in this scope /home/milian/projects/qt5/qtwebkit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/DirectiveParser.cpp:771: error: 'atoi' was not declared in this scope cc: /opt/qnx650/host/linux/x86/usr/lib/gcc/arm-unknown-nto-qnx6.5.0eabi/4.4.2/cc1plus error 1 make[2]: *** [obj/debug/src/compiler/preprocessor/new/DirectiveParser.o] Error 1 make[2]: Leaving directory `/home/milian/projects/qt5/build-qnx650/qtwebkit/Source/ThirdParty/ANGLE' Where would you add the include to string.h? Into MacroExpander.h? DirectiveParser.cpp? But then again, if I'm patching ANGLE sources, I can also fix it by adding the std:: namespace identifier. Anyhow, I'll ask the ANGLE people on what they prefer, so far noone has taken an interest in my patch.

> [13:30] <tronical> milian: I suppose an implicit inclusion of string.h from > somewhere, so that the extern "C" variant of strlen is chosen? string.h isn't being included. What happens is that some compilers, when including the cstring, cstdlib etc, will also define the stuff in global namespace. QNX's compiler does'n do this, but it isn't a compiler bug, as the standard doesn't specify: http://stackoverflow.com/questions/9149368/is-it-ok-to-qualify-c-functions-with-the-std-namespace > > Wouldn't a simpler way to fix this would be to add one or more includes of <string.h>? > > Millian, did you see Darin's comment? If that works then it would seem like a much simpler solution :) Build also fails on malloc(), we would need stdlib.h too, we're opening a can of worms. IMO, duplicating C and C++ headers is not a good solution. Besides, C header usage in C++ is discouraged by the standard. A couple of "using" or std:: would be best.

Created attachment 174978 [details] Patch Updated patch. I'll still try to resolve this issue somehow in ANGLE upstream...

Upstream ANGLE diverged quite a bit, I've submitted a patch but I'm not sure whether that is all that is required to get it working for WebKit. See: https://codereview.appspot.com/6843083/

Ok, my patch was accepted upstream in ANGLE. What do I do now to get this issue resolved in WebKit? Do I have to submit a patch that updates the local checkout of ANGLE in the WebKit source tree? See: https://codereview.appspot.com/6843083/ Considering that upstream ANGLE diverged, we cannot apply the patch from there 1to1 to the WebKit copy.

I've seen patches against the copy of ANGLE in WebKit, but I'm not sure that's the right way. CC'ing a couple of people who might know how updates are done.

@dino did the last major update of the copy of ANGLE in WebKit.

(In reply to comment #16) > @dino did the last major update of the copy of ANGLE in WebKit. WebKit ANGLE diverged from upstream ANGLE, but maybe we can sync back up now. I'll look into this.

(In reply to comment #17) > (In reply to comment #16) > > @dino did the last major update of the copy of ANGLE in WebKit. > > WebKit ANGLE diverged from upstream ANGLE, but maybe we can sync back up now. I'll look into this. If this is urgent, and it is already landed in upstream, then you can apply to the WebKit fork. The goal is to get back onto the official ANGLE as soon as possible, so that we can avoid these situations.

The QT port is no longer part of WebKit.