Created attachment 48563 [details] Fix v1

Attachment 48563 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Ignoring "JavaScriptCore/qt/api/qscriptstring.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/QtScript.pro": this file is exempt from the style guide. JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:23: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:24: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Ignoring "JavaScriptCore/qt/api/qscriptengine_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.cpp": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide. Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 48563 [details] Fix v1 > +bool QScriptString::operator==(const QScriptString& other) const > +{ > + return d_ptr == other.d_ptr ? true : *d_ptr == *(other.d_ptr); I personally find it easier to read the following: return d_ptr == other.d_ptr || *d_ptr == *other.d_ptr; > +class QScriptStringPrivate : public QSharedData { > +public: > + inline QScriptStringPrivate(); > + inline QScriptStringPrivate(const QString& qtstring); > + inline ~QScriptStringPrivate(); > + > + static inline QScriptString get(QScriptStringPrivate* d); > + static inline QScriptStringPtr get(const QScriptString& p); > + > + inline bool isValid() const; > + > + inline bool operator==(const QScriptStringPrivate& other) const; > + inline bool operator!=(const QScriptStringPrivate& other) const; > + > + inline quint32 toArrayIndex(bool* ok = 0) const; > + > + inline QString toString() const; > + > + inline quint64 id() const; > + > +private: > + JSStringRef m_jsstring; > + QString m_qtstring; > +}; I don't think we should store a QString here. If the goal is to speed up toArrayIndex(), then I suggest to either add the functionality to the C API or by implementing it on top without the expensive conversion by for example using JSStringGetCharactersPtr.

Created attachment 48803 [details] Fix v2 (In reply to comment #3) > (From update of attachment 48563 [details]) > > > +bool QScriptString::operator==(const QScriptString& other) const > > +{ > > + return d_ptr == other.d_ptr ? true : *d_ptr == *(other.d_ptr); > > I personally find it easier to read the following: > > return d_ptr == other.d_ptr || *d_ptr == *other.d_ptr; Fixed (in both; == and != operators). > > +class QScriptStringPrivate : public QSharedData { > > (...) > > + JSStringRef m_jsstring; > > + QString m_qtstring; > > +}; > > I don't think we should store a QString here. > > If the goal is to speed up toArrayIndex(), then I suggest to either add the > functionality to the C API or by implementing it on top without the expensive > conversion by for example using JSStringGetCharactersPtr. I replaced the QString by a quint32. In this implementation conversion is needed only once. It is not possible to avoid it without JSC C API changes.

Attachment 48803 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Ignoring "JavaScriptCore/qt/api/qscriptconverter_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/QtScript.pro": this file is exempt from the style guide. JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:23: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:24: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Ignoring "JavaScriptCore/qt/api/qscriptengine_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.cpp": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide. Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 49065 [details] Fix v3 Export macro was added. toArrayIndex() caching was removed.

Attachment 49065 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Ignoring "JavaScriptCore/qt/api/qscriptconverter_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/QtScript.pro": this file is exempt from the style guide. JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:23: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:24: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Ignoring "JavaScriptCore/qt/api/qscriptengine_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.cpp": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide. Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Jedrzej, shouldn't you fix the style error?

(In reply to comment #8) > Jedrzej, shouldn't you fix the style error? I believe that they are false positives. I registered two bugs; 35143 and 35151.

Created attachment 49286 [details] Fix v4 Changelog: Algorithm was simplified. Code was moved to the QScriptConverter class (there is high probability that it will be reused from QScriptValue). New test cases were added. Fix conversion from float (ex. 0.3 is not a correct index array).

Attachment 49286 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Ignoring "JavaScriptCore/qt/api/qscriptconverter_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/QtScript.pro": this file is exempt from the style guide. JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:23: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] JavaScriptCore/qt/tests/qscriptstring/tst_qscriptstring.cpp:24: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Ignoring "JavaScriptCore/qt/api/qscriptengine_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.cpp": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptengine.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring_p.h": this file is exempt from the style guide. Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide. Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 49286 [details] Fix v4 Clearing flags on attachment: 49286 Committed r55426: <http://trac.webkit.org/changeset/55426>

All reviewed patches have been landed. Closing bug.