GitHub PullRequestReviewCommentEvent at 2014-04-14 20:50:34
Repository: dolphin
Actor: delroth
Payload: {"comment":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/pulls/comments/11607146","id":11607146,"diff_hunk":"@@ -171,3 +174,186 @@ struct BitField\n \tstatic_assert(bits > 0, \"Invalid number of bits\");\n };\n #pragma pack()\n+\n+/*\n+ * Allows grouping multiple BitField objects into an array\n+ *\n+ * Get() behaves exactly like the [] operator of a regular array, but\n+ * it can only be used if the array index is known at compile time (as a\n+ * constexpr). If that is not the case, GetValue and SetValue allow modifying\n+ * the bitfields. Do note that the latter functions might incur a minimal\n+ * performance overhead, especially if the compiler cannot determine the\n+ * index value on compile time.\n+ *\n+ * Usage:\n+ *\n+ * Given a union (or structure) called SomeStructure containing any BitFields,\n+ * the bitfields can be grouped by adding a new method returning a\n+ * BitFieldArray object, e.g.:\n+ *\n+ * struct SomeStructure\n+ * {\n+ *     u32 hex;\n+ *     BitField<0, 8, u32> field1;\n+ *     BitField<8, 2, u32> field2;\n+ *     BitField<10, 5, u32> field3;\n+ *\n+ *     DECLARE_BITFIELD_ARRAY(GetFieldArray, field1, field2, field3);\n+ * };\n+ *\n+ */\n+template<typename... BitFields>\n+class BitFieldArray\n+{\n+private:\n+\t// TODO: Assert that all tuple bitfields use the same underlying type\n+\tstd::tuple<BitFields&...> bitfields;\n+\n+\t// Helper functions to create sub-tuples\n+\ttemplate<unsigned...s> struct seq { typedef seq<s...>& type; };\n+\ttemplate<unsigned max, unsigned... s> struct make_seq:make_seq<max-1, max-1, s...> {};\n+\ttemplate<unsigned...s> struct make_seq<0, s...>:seq<s...> {};\n+\n+\ttemplate<unsigned... s, typename Tuple>\n+\tstatic auto extract_tuple(seq<s...>, Tuple& tup) -> decltype(std::tie(std::get
(tup)...))\n+\t{\n+\t\treturn std::tie(std::get
(tup)...);\n+\t}\n+\n+\t// ArrayElement: Private variadic template class used to deal with indexable tuples with the common base type UnderlyingType.\n+\t// Implemented via recursion, with the common members stored in ArrayElementBase.\n+\ttemplate<typename... BitFields_2>\n+\tclass ArrayElementBase\n+\t{\n+\tprotected:\n+\t\tArrayElementBase(size_t idx, std::tuple<BitFields_2&...> bfs) : index(idx), fields(bfs) {}\n+\n+\t\tconst size_t index;\n+\t\tconst std::tuple<BitFields_2&...> fields;\n+\t\ttypedef typename std::tuple_element<0,std::tuple<BitFields_2...>>::type::UnderlyingType UnderlyingType;\n+\t};\n+\n+\ttemplate<typename B1_2, typename... BitFields_2>\n+\tclass ArrayElement : public ArrayElementBase<B1_2, BitFields_2...>\n+\t{\n+\t\ttypedef typename ArrayElementBase<B1_2, BitFields_2...>::UnderlyingType UnderlyingType;\n+\n+//\t\tstatic_assert(typeid(typename std::tuple_element<0,std::tuple<BitFields_2...>>::type::UnderlyingType) ==\n+//\t\t              typeid(typename std::tuple_element<1,std::tuple<BitFields_2...>>::type::UnderlyingType), \"Typeids mismatch!\");\n+\n+\tpublic:\n+\t\tArrayElement(size_t idx, std::tuple<B1_2&, BitFields_2&...> bfs) : ArrayElementBase<B1_2, BitFields_2...>(idx, bfs)\n+\t\t{\n+\t\t}\n+\n+\t\t// TODO: Not sure if it's a good idea to expose this operator.\n+\t\t__forceinline ArrayElement& operator=(const ArrayElement& other)\n+\t\t{\n+\t\t\t*this = (UnderlyingType)other;\n+\t\t\treturn *this;\n+\t\t}\n+\n+\t\t__forceinline ArrayElement& operator=(const UnderlyingType& val)\n+\t\t{\n+\t\t\t// TODO: I don't get it. shouldn't this be == 0?\n+\t\t\tif (this->index == sizeof...(BitFields_2))\n+\t\t\t{\n+\t\t\t\tstd::get<sizeof...(BitFields_2)>(this->fields) = val;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t{\n+\t\t\t\tMakeArrayElementFromTuple(this->index, extract_tuple(make_seq<sizeof...(BitFields_2)>(), this->fields)) = val;\n+\t\t\t}\n+\t\t\treturn *this;\n+\t\t}\n+\n+\t\t__forceinline operator UnderlyingType() const\n+\t\t{\n+\t\t\tif (this->index == sizeof...(BitFields_2))\n+\t\t\t{\n+\t\t\t\treturn std::get<sizeof...(BitFields_2)>(this->fields);\n+\t\t\t}\n+\t\t\telse\n+\t\t\t{\n+\t\t\t\tauto sub_tuple = extract_tuple(make_seq<sizeof...(BitFields_2)>(), this->fields);\n+\t\t\t\treturn MakeArrayElementFromTuple(this->index, sub_tuple);\n+\t\t\t}\n+\t\t}\n+\t};\n+\n+\ttemplate<typename B1_2>\n+\tclass ArrayElement<B1_2> : public ArrayElementBase<B1_2>\n+\t{\n+\t\ttypedef typename ArrayElementBase<B1_2>::UnderlyingType UnderlyingType;\n+\n+\tpublic:\n+\t\tArrayElement(size_t idx, std::tuple<B1_2&> bfs) : ArrayElementBase<B1_2>(idx, bfs)\n+\t\t{\n+\t\t}\n+\n+\t\t// TODO: Not sure if it's a good idea to expose this operator.\n+\t\t__forceinline ArrayElement& operator=(const ArrayElement& other)\n+\t\t{\n+\t\t\t*this = (UnderlyingType)other;\n+\t\t\treturn *this;\n+\t\t}\n+\n+\t\t__forceinline ArrayElement& operator=(UnderlyingType val)\n+\t\t{\n+\t\t\tstd::get<0>(this->fields) = val;\n+\t\t\treturn *this;\n+\t\t}\n+\n+\t\t__forceinline operator UnderlyingType() const\n+\t\t{\n+\t\t\treturn std::get<0>(this->fields);\n+\t\t}\n+\t};\n+\n+\ttemplate<typename... BitFields_2>\n+\tstatic ArrayElement<BitFields_2...> MakeArrayElementFromTuple(size_t index, std::tuple<BitFields_2&...> bfs)\n+\t{\n+\t\treturn ArrayElement<BitFields_2...>(index, bfs);\n+\t}\n+\n+public:\n+\tBitFieldArray(std::tuple<BitFields&...> tup) : bitfields(tup)","path":"Source/Core/Common/BitField.h","position":164,"original_position":164,"commit_id":"028128c0115f78dd9f61910199513ff2fd5b6091","original_commit_id":"028128c0115f78dd9f61910199513ff2fd5b6091","user":{"login":"delroth","id":202798,"avatar_url":"https://avatars.githubusercontent.com/u/202798?","gravatar_id":"a7c333ee5d59519be7b757f917a950ba","url":"https://api.github.com/users/delroth","html_url":"https://github.com/delroth","followers_url":"https://api.github.com/users/delroth/followers","following_url":"https://api.github.com/users/delroth/following{/other_user}","gists_url":"https://api.github.com/users/delroth/gists{/gist_id}","starred_url":"https://api.github.com/users/delroth/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/delroth/subscriptions","organizations_url":"https://api.github.com/users/delroth/orgs","repos_url":"https://api.github.com/users/delroth/repos","events_url":"https://api.github.com/users/delroth/events{/privacy}","received_events_url":"https://api.github.com/users/delroth/received_events","type":"User","site_admin":false},"body":"Please use the m_ convention for class members. This code is already confusing enough, let's not make it less readable by using the same namespace for class members and variables :)","created_at":"2014-04-14T20:50:34Z","updated_at":"2014-04-14T20:50:34Z","html_url":"https://github.com/dolphin-emu/dolphin/pull/273#discussion_r11607146","pull_request_url":"https://api.github.com/repos/dolphin-emu/dolphin/pulls/273","_links":{"self":{"href":"https://api.github.com/repos/dolphin-emu/dolphin/pulls/comments/11607146"},"html":{"href":"https://github.com/dolphin-emu/dolphin/pull/273#discussion_r11607146"},"pull_request":{"href":"https://api.github.com/repos/dolphin-emu/dolphin/pulls/273"}}}}