Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469323002:
Document TaskRunners/TaskTraits/FROM_HERE as implicitly included from post_task.h (Closed)
DescriptionDocument TaskRunners/TaskTraits/FROM_HERE as implicitly included from post_task.h
This avoids the majority of simple callsites to have to include task_traits.h +
location.h + one of the task_runner.h headers merely to use post_task.h.
BUG=633389
Patch Set 1 #
Total comments: 2
Patch Set 2 : Keep header ordering, add comment after #
Total comments: 3
Messages
Total messages: 13 (3 generated)
Description was changed from ========== Document TaskRunners/TaskTraits/FROM_HERE as implicitly included from post_task.h BUG=633389 ========== to ========== Document TaskRunners/TaskTraits/FROM_HERE as implicitly included from post_task.h This avoids the majority of simple callsites to have to include task_traits.h + location.h + one of the task_runner.h headers merely to use post_task.h. BUG=633389 ==========
gab@chromium.org changed reviewers: + fdoray@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2469323002/diff/1/base/task_scheduler/post_ta... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2469323002/diff/1/base/task_scheduler/post_ta... base/task_scheduler/post_task.h:12: // To avoid redundant includes: users may omit including the following headers https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes The language in the style guide does not seem to permit deviation. Keep this in alphabetical order and then have a comment banner below discussing the permitted files to omit.
https://codereview.chromium.org/2469323002/diff/1/base/task_scheduler/post_ta... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2469323002/diff/1/base/task_scheduler/post_ta... base/task_scheduler/post_task.h:12: // To avoid redundant includes: users may omit including the following headers On 2016/11/02 17:06:25, robliao wrote: > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > The language in the style guide does not seem to permit deviation. > > > Keep this in alphabetical order and then have a comment banner below discussing > the permitted files to omit. Done.
lgtm Maybe add ref_counted.h to the list? // It would be weird if I had to include ref_counted.h but not task_runner.h. scoped_refptr<TaskRunner> task_runner = CreateTaskRunnerWithTraits(TaskTraits());
lgtm https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... base/task_scheduler/post_task.h:19: // with this API: location.h, task_traits.h, and *task_runner.h. Is the asterisk in task_runner.h intentional?
On 2016/11/02 20:08:14, fdoray wrote: > lgtm > > Maybe add ref_counted.h to the list? > > // It would be weird if I had to include ref_counted.h but not task_runner.h. > > scoped_refptr<TaskRunner> task_runner = > CreateTaskRunnerWithTraits(TaskTraits()); My thinking is I want to allow simple inline calls (e.g. either inline PostTask calls or inline GetTaskRunner and implicitly move it to another owner). If you want to keep ownership, then I guess it makes sense that you would include ref_counted.h? But then I guess that by my latest discovery about ref_counted.h means you should also include the matching task_runner.h header... I guess you're right but I find it weird to say that it comes with ref_counted.h -- e.g. what if TaskRunner is one day no longer refcounted?
https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... base/task_scheduler/post_task.h:19: // with this API: location.h, task_traits.h, and *task_runner.h. On 2016/11/02 20:13:55, robliao wrote: > Is the asterisk in task_runner.h intentional? Yes, it applies to all 3 TaskRunner headers.
gab@chromium.org changed reviewers: + danakj@chromium.org
+danakj, in particular WDYT of the discussion below? On 2016/11/02 20:24:43, gab wrote: > On 2016/11/02 20:08:14, fdoray wrote: > > lgtm > > > > Maybe add ref_counted.h to the list? > > > > // It would be weird if I had to include ref_counted.h but not task_runner.h. > > > > scoped_refptr<TaskRunner> task_runner = > > CreateTaskRunnerWithTraits(TaskTraits()); > > My thinking is I want to allow simple inline calls (e.g. either inline PostTask > calls or inline GetTaskRunner and implicitly move it to another owner). If you > want to keep ownership, then I guess it makes sense that you would include > ref_counted.h? > > But then I guess that by my latest discovery about ref_counted.h means you > should also include the matching task_runner.h header... > > I guess you're right but I find it weird to say that it comes with ref_counted.h > -- e.g. what if TaskRunner is one day no longer refcounted?
On 2016/11/02 20:25:59, gab wrote: > +danakj, in particular WDYT of the discussion below? > > On 2016/11/02 20:24:43, gab wrote: > > On 2016/11/02 20:08:14, fdoray wrote: > > > lgtm > > > > > > Maybe add ref_counted.h to the list? > > > > > > // It would be weird if I had to include ref_counted.h but not > task_runner.h. > > > > > > scoped_refptr<TaskRunner> task_runner = > > > CreateTaskRunnerWithTraits(TaskTraits()); > > > > My thinking is I want to allow simple inline calls (e.g. either inline > PostTask > > calls or inline GetTaskRunner and implicitly move it to another owner). If you > > want to keep ownership, then I guess it makes sense that you would include > > ref_counted.h? > > > > But then I guess that by my latest discovery about ref_counted.h means you > > should also include the matching task_runner.h header... > > > > I guess you're right but I find it weird to say that it comes with > ref_counted.h > > -- e.g. what if TaskRunner is one day no longer refcounted? I think it's more or less impossible to get it right when it's not automated and you shouldn't lose too much sleep over it, and you should just drop the comment and let people include what they will. https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... File base/task_scheduler/post_task.h (right): https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... base/task_scheduler/post_task.h:19: // with this API: location.h, task_traits.h, and *task_runner.h. On 2016/11/02 20:25:21, gab wrote: > On 2016/11/02 20:13:55, robliao wrote: > > Is the asterisk in task_runner.h intentional? > > Yes, it applies to all 3 TaskRunner headers. I think this comment is overprescribing things, just use the right includes and let users do as they will.
On 2016/11/02 22:58:01, danakj_OOO_back_11_9 wrote: > On 2016/11/02 20:25:59, gab wrote: > > +danakj, in particular WDYT of the discussion below? > > > > On 2016/11/02 20:24:43, gab wrote: > > > On 2016/11/02 20:08:14, fdoray wrote: > > > > lgtm > > > > > > > > Maybe add ref_counted.h to the list? > > > > > > > > // It would be weird if I had to include ref_counted.h but not > > task_runner.h. > > > > > > > > scoped_refptr<TaskRunner> task_runner = > > > > CreateTaskRunnerWithTraits(TaskTraits()); > > > > > > My thinking is I want to allow simple inline calls (e.g. either inline > > PostTask > > > calls or inline GetTaskRunner and implicitly move it to another owner). If > you > > > want to keep ownership, then I guess it makes sense that you would include > > > ref_counted.h? > > > > > > But then I guess that by my latest discovery about ref_counted.h means you > > > should also include the matching task_runner.h header... > > > > > > I guess you're right but I find it weird to say that it comes with > > ref_counted.h > > > -- e.g. what if TaskRunner is one day no longer refcounted? > > I think it's more or less impossible to get it right when it's not automated and > you shouldn't lose too much sleep over it, and you should just drop the comment > and let people include what they will. > > https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... > File base/task_scheduler/post_task.h (right): > > https://codereview.chromium.org/2469323002/diff/20001/base/task_scheduler/pos... > base/task_scheduler/post_task.h:19: // with this API: location.h, task_traits.h, > and *task_runner.h. > On 2016/11/02 20:25:21, gab wrote: > > On 2016/11/02 20:13:55, robliao wrote: > > > Is the asterisk in task_runner.h intentional? > > > > Yes, it applies to all 3 TaskRunner headers. > > I think this comment is overprescribing things, just use the right includes and > let users do as they will. Ok, I guess what I'm trying to prevent is someone making a simple call and being told to IWYU 4 different headers... IMO it's fine to only include post_task.h when using TaskTraits/FROM_HERE/TaskRunner in its context only. Closing this for now. |
