CodeGeneratorInspector.py largely contains multiline string literals. Those should be taken away into a separate file to keep code and string resources better separated.
Created attachment 188951 [details] Patch
Comment on attachment 188951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188951&action=review > Source/WebCore/inspector/CodeGeneratorInspector.py:40 > +import CodeGeneratorInspectorStrings Maybe CodeGeneratorInspectorTemplates?
View in context: https://bugs.webkit.org/attachment.cgi?id=188951&action=review What do you think about further splitting of this file. I'd expect to see one template file per one result file. > Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:701 > +#endif // !ASSERT_DISABLED Wrong comment here and in many other places like that. > Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:922 > +class_binding_builder_part_1 = ( > +""" AllFieldsSet = %s > + }; > + > + template<int STATE> > + class Builder { > + private: > + RefPtr<InspectorObject> m_result; > + > + template<int STEP> Builder<STATE | STEP>& castState() > + { > + return *reinterpret_cast<Builder<STATE | STEP>*>(this); > + } > + > + Builder(PassRefPtr</*%s*/InspectorObject> ptr) > + { > + COMPILE_ASSERT(STATE == NoFieldsSet, builder_created_in_non_init_state); > + m_result = ptr; > + } > + friend class %s; > + public: > +""") I think that it would be more declarative and less error prone if you convert it to a function with named argument instead of a string.
> > Source/WebCore/inspector/CodeGeneratorInspector.py:40 > > +import CodeGeneratorInspectorStrings > Maybe CodeGeneratorInspectorTemplates? Please write how positive you are about this change, because I'm 50/50. The reason why I chose "strings" is because I meant this file as plain storage for multiline strings. I don't care whether they are templates or what. Multiline string is the only thing you cannot take with you into type-safe language if you ever want to.
> What do you think about further splitting of this file. I'd expect to see one template file per one result file. That would bring more strange files into repository, but I don't see a clear benefit from this. You are not going to get a real general-purpose template engine anyway. Current code structure is very traceable. For example, all generated scripts declare the path to the generator file. > > Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:701 > > +#endif // !ASSERT_DISABLED > Wrong comment here and in many other places like that. Thanks. > > Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:922 > > +class_binding_builder_part_1 = ( > > +""" AllFieldsSet = %s > > + }; > > + > > + template<int STATE> > > + class Builder { > > + private: > > + RefPtr<InspectorObject> m_result; > > + > > + template<int STEP> Builder<STATE | STEP>& castState() > > + { > > + return *reinterpret_cast<Builder<STATE | STEP>*>(this); > > + } > > + > > + Builder(PassRefPtr</*%s*/InspectorObject> ptr) > > + { > > + COMPILE_ASSERT(STATE == NoFieldsSet, builder_created_in_non_init_state); > > + m_result = ptr; > > + } > > + friend class %s; > > + public: > > +""") > > I think that it would be more declarative and less error prone if you convert it to a function with named argument instead of a string. Unfortunately that's not what I planned to do. I see at as a collection of multiline string resources, no code or any other mark-up. If I ever were to switch script to type-safe language, I would need multiline strings separate. In this form as I suggest, I would be able to write a really simple RegExp-based parser to easily read this strings. This is basically a pseudo-python similar to like JSON is a pseuod-JavaScript.
Created attachment 189164 [details] Patch
(In reply to comment #4) > > > Source/WebCore/inspector/CodeGeneratorInspector.py:40 > > > +import CodeGeneratorInspectorStrings > > Maybe CodeGeneratorInspectorTemplates? > > Please write how positive you are about this change, because I'm 50/50. > > The reason why I chose "strings" is because I meant this file as plain storage for multiline strings. I don't care whether they are templates or what. > > Multiline string is the only thing you cannot take with you into type-safe language if you ever want to. Ok, that makes sense.
Comment on attachment 189164 [details] Patch Clearing flags on attachment: 189164 Committed r143586: <http://trac.webkit.org/changeset/143586>
All reviewed patches have been landed. Closing bug.