The column number for function block entries can be erroneous. Will fix. ref: <rdar://problem/13277445>.
Created attachment 193974 [details] the patch
Attachment 193974 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/StringImpl.cpp', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1 Source/JavaScriptCore/parser/Parser.h:975: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 193974 [details] the patch Attachment 193974 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17213721
Created attachment 193980 [details] new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment.
Attachment 193980 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.order', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreExports.def', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCoreExportGenerator/JavaScriptCoreExports.def.in', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/StringImpl.cpp', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1 Source/JavaScriptCore/parser/Parser.h:975: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 193980 [details] new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment. View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review > Source/JavaScriptCore/interpreter/Interpreter.cpp:1347 > + size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition); Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line? e.g. <script>foo() </script> or <foo onload="blah()"> ? > Source/WTF/ChangeLog:18 > +2013-03-19 Mark Lam <mark.lam@apple.com> > + > + Need a short description (OOPS!). > + Need the bug URL (OOPS!). > + > + Reviewed by NOBODY (OOPS!). > + > + * wtf/text/StringImpl.cpp: > + (WTF::StringImpl::reverseFindLineTerminator): > + (WTF): > + * wtf/text/StringImpl.h: > + (StringImpl): > + (WTF): > + (WTF::reverseFindLineTerminator): > + * wtf/text/WTFString.h: > + (WTF::String::reverseFindLineTerminator): > + (String): > + Double changelog :(
Comment on attachment 193980 [details] new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment. View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review >> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347 >> + size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition); > > Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line? e.g. > > <script>foo() > </script> > > or <foo onload="blah()"> > ? It checks for a ‘\n’ or ‘\t’ or ‘\0’. So, it doesn’t care if you have html tags preceding it. It truly measures the displacement from the start of the line. That’s what we want, right? >> Source/WTF/ChangeLog:18 >> + > > Double changelog :( Duh!!! Will fix. Can I get a r+ to land assuming I fix this before landing?
(In reply to comment #7) > (From update of attachment 193980 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review > > >> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347 > >> + size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition); > > > > Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line? e.g. > > > > <script>foo() > > </script> > > > > or <foo onload="blah()"> > > ? > > It checks for a ‘\n’ or ‘\t’ or ‘\0’. So, it doesn’t care if you have html tags preceding it. It truly measures the displacement from the start of the line. That’s what we want, right? '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line? Alternatively how do you avoid walking off the start of the source?
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 193980 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review > > > > >> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347 > > >> + size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition); > > > > > > Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line? e.g. > > > > > > <script>foo() > > > </script> > > > > > > or <foo onload="blah()"> > > > ? > > > > It checks for a ‘\n’ or ‘\t’ or ‘\0’. So, it doesn’t care if you have html tags preceding it. It truly measures the displacement from the start of the line. That’s what we want, right? > > '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line? Alternatively how do you avoid walking off the start of the source? See reverseFindLineTerminator() in StringImpl.h (also in this patch). “source” is a WTF::String. The reverseFind is bounded by the length of the string. If it runs out of characters, it will return notFound.
> (In reply to comment #8) > > '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line? Alternatively how do you avoid walking off the start of the source? Regarding ‘\0’, I included in the StringImpl::reverseFindLineTerminator() because it uses an explicitly specified length unlike C strings that requires a ‘\0’ terminator. I wasn’t sure if we ever allow WTF::String instances that include ‘\0’ in them (though we don’t allow that in our parsed source strings. Are you saying that we (the webkit project) does not allow that? If so, I can remove the ‘\0’ check in StringImpl::reverseFindLineTerminator().
(In reply to comment #10) > > (In reply to comment #8) > > > '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line? Alternatively how do you avoid walking off the start of the source? > > Regarding ‘\0’, I included in the StringImpl::reverseFindLineTerminator() because it uses an explicitly specified length unlike C strings that requires a ‘\0’ terminator. I wasn’t sure if we ever allow WTF::String instances that include ‘\0’ in them (though we don’t allow that in our parsed source strings. Are you saying that we (the webkit project) does not allow that? If so, I can remove the ‘\0’ check in StringImpl::reverseFindLineTerminator(). It's probably possible to induce a null in a html string or some such, but in such a case i don't believe the null would be a line delimiter
Created attachment 194000 [details] patch : addressed Oliver’s feedback.
Attachment 194000 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.order', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreExports.def', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCoreExportGenerator/JavaScriptCoreExports.def.in', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/StringImpl.cpp', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1 Source/JavaScriptCore/parser/Parser.h:975: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for the review. Landed in r146318: <http://trac.webkit.org/changeset/146318>.