Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2938613002:
Add a variadic version of ParamTraits, to allow for >5 IPC parameters. (Closed)
|
Created:
3 years, 6 months ago by ossu-chromium Modified:
3 years, 6 months ago Reviewers:
Ken Rockot(use gerrit already) CC:
chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a variadic version of ParamTraits, to allow for >5 IPC parameters.
I needed to add one more parameter to AudioInputMsg_NotifyStreamCreated
and ran into this limit.
BUG=chromium:729002
Review-Url: https://codereview.chromium.org/2938613002
Cr-Commit-Position: refs/heads/master@{#481491}
Committed: https://chromium.googlesource.com/chromium/src/+/fdd19a3a59659ea485633fe69005ca2b8c4d7ea5
Patch Set 1 #Patch Set 2 : Fixed GetSize implementation. #
Total comments: 2
Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Added Tuple to the helper name. Simplified end conditions. #
Total comments: 2
Patch Set 5 : Some style changes. #Messages
Total messages: 33 (25 generated)
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ossu@chromium.org changed reviewers: + rockot@chromium.org
PTAL. I was working on a CL that took me over the limit of 5 IPC parameters, so I had to add support for more. Decided to make ParamTraits variadic. https://codereview.chromium.org/2938613002/diff/40001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/2938613002/diff/40001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:756: struct ParamTraitsHelper<I, I> { Partial specialization for when I == MAX. https://codereview.chromium.org/2938613002/diff/40001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:797: static void Log(const param_type& p, std::string* l) { Helper::Log(p, l); } clang-format put this on one line... I'm not sure I like it.
Cool - I actually wrote something like this recently but scrapped it because I ended up not needing my 6th arg after all. :) I don't think we need a generic ParamTraitsHelper<T> - this should be a TupleParamTraitsHelper specifically IMHO. Some other comments inline which I think should reduce the code length and/or improve clarity a bit. https://codereview.chromium.org/2938613002/diff/80001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/2938613002/diff/80001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:727: template <int I, int MAX> How about counting down instead of counting up? This should read more simply: template <int ArgIndex> struct TupleParamTraitsHelper { template <T> ... GetSize... { ParamTraitsHelper<ArgIndex - 1>::GetSize(sizer, p); GetParamSize(sizer, std::get<ArgIndex - 1>(p)); } // ... } and then you simply specialize <0> to do nothing in each method. https://codereview.chromium.org/2938613002/diff/80001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:729: template <typename T> Not sure there's much reason to template each method separately? i.e. why not just have: template <int ArgIndex, typename... TupleArgs> strict TupleParamTraitsHelper { using T = std::tuple<TupleArgs...>; // ... };
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Renamed to TupleParamTraitsHelper and cleaned up the implementation a bit. https://codereview.chromium.org/2938613002/diff/80001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/2938613002/diff/80001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:727: template <int I, int MAX> On 2017/06/14 17:42:26, Ken Rockot(use gerrit already) wrote: > How about counting down instead of counting up? This should read more simply: > > template <int ArgIndex> > struct TupleParamTraitsHelper { > template <T> > ... GetSize... { > ParamTraitsHelper<ArgIndex - 1>::GetSize(sizer, p); > GetParamSize(sizer, std::get<ArgIndex - 1>(p)); > } > // ... > } > > and then you simply specialize <0> to do nothing in each method. I tried it and I think my new version reads a bit clearer, although perhaps being slightly longer, than the pre-order recursive one. I've kept it counting up, but aliased the helper for the next step and made the end case no-ops. https://codereview.chromium.org/2938613002/diff/80001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:729: template <typename T> On 2017/06/14 17:42:27, Ken Rockot(use gerrit already) wrote: > Not sure there's much reason to template each method separately? i.e. why not > just have: > > template <int ArgIndex, typename... TupleArgs> > strict TupleParamTraitsHelper { > using T = std::tuple<TupleArgs...>; > // ... > }; Right. I've just put the type as a third class template argument.
LGTM! https://codereview.chromium.org/2938613002/diff/100001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/2938613002/diff/100001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:727: template <typename T, int I, int COUNT> super nitty nit: The style guide seems to be silent on this issue, but I would prefer that this code use normal naming conventions (i.e., typenames are CamelCase, variable names are snake_case) for the template arg names. So something like: template <typename T, int index, int count> instead of the LOUDCASE used here. https://codereview.chromium.org/2938613002/diff/100001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:763: template <class... Args> nit: for consistency with most new code please consider writing typename... instead of class...
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ossu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2938613002/#ps120001 (title: "Some style changes.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ossu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498122046423720,
"parent_rev": "25ff1f1151352353b5c30a719a833c8a3f1dc05e", "commit_rev":
"fdd19a3a59659ea485633fe69005ca2b8c4d7ea5"}
Message was sent while issue was closed.
Description was changed from ========== Add a variadic version of ParamTraits, to allow for >5 IPC parameters. I needed to add one more parameter to AudioInputMsg_NotifyStreamCreated and ran into this limit. BUG=chromium:729002 ========== to ========== Add a variadic version of ParamTraits, to allow for >5 IPC parameters. I needed to add one more parameter to AudioInputMsg_NotifyStreamCreated and ran into this limit. BUG=chromium:729002 Review-Url: https://codereview.chromium.org/2938613002 Cr-Commit-Position: refs/heads/master@{#481491} Committed: https://chromium.googlesource.com/chromium/src/+/fdd19a3a59659ea485633fe69005... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fdd19a3a59659ea485633fe69005... |
