Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2423423002:
Explicitly Document base/bind_helpers.h Comes with base/bind.h (Closed)
DescriptionExplicitly Document base/bind_helpers.h Comes with base/bind.h
It's hard to use base/bind.h without base/bind_helpers.h.
This comment explicitly demonstrates intent to include base_helpers.h to satisfy
the intent requirement in the C++ style guide.
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
BUG=
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update Wording #Messages
Total messages: 20 (4 generated)
Description was changed from ========== Explicitly Document base/bind_helpers.h Comes with base/bind.h BUG= ========== to ========== Explicitly Document base/bind_helpers.h Comes with base/bind.h It's hard to use base/bind.h without base/bind_helpers.h. BUG= ==========
Description was changed from ========== Explicitly Document base/bind_helpers.h Comes with base/bind.h It's hard to use base/bind.h without base/bind_helpers.h. BUG= ========== to ========== Explicitly Document base/bind_helpers.h Comes with base/bind.h It's hard to use base/bind.h without base/bind_helpers.h. This comment explicitly demonstrates intent to include base_helpers.h to satisfy the intent requirement in the C++ style guide. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes BUG= ==========
robliao@chromium.org changed reviewers: + dcheng@chromium.org
Is the motivation behind this to avoid having to include both bind.h and bind_helpers.h? https://codereview.chromium.org/2423423002/diff/1/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2423423002/diff/1/base/bind.h#newcode15 base/bind.h:15: // It's not necessary to include base/bind_helpers.h in your implementation. Nit: "your implementation" feels a little vague here: this can be read to mean the implementation of base::Bind.
> Is the motivation behind this to avoid having to include both bind.h and bind_helpers.h? That is the exact desire. https://codereview.chromium.org/2423423002/diff/1/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2423423002/diff/1/base/bind.h#newcode15 base/bind.h:15: // It's not necessary to include base/bind_helpers.h in your implementation. On 2016/10/18 19:30:56, dcheng wrote: > Nit: "your implementation" feels a little vague here: this can be read to mean > the implementation of base::Bind. Updated to "your file".
I'd like to do some code archaeology to understand the reason for the original split. I suspect it's mostly because we used to use pump to generate callback.h and bind.h, but I want to make sure there wasn't anything else. Out of curiosity, does this cause a lot of issues in code reviews?
On 2016/10/18 19:54:17, dcheng wrote: > I'd like to do some code archaeology to understand the reason for the original > split. I suspect it's mostly because we used to use pump to generate callback.h > and bind.h, but I want to make sure there wasn't anything else. > > Out of curiosity, does this cause a lot of issues in code reviews? Some. Mostly around the include what you use desire. //docs/callback.md implies that these functions are there by just using them without mentioning that you also need to include bind_helpers.h.
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
lgtm
So I have a proposal, which I hope will make everyone happy (but is a bit more complicated). We should just move Passed(), Unretained(), IgnoreResult(), etc. into bind.h. The support helpers for those should move into bind_internal.h (I don't think there's anyone that's ever looked at bind_helpers.h hoping to find the implementation of PassedWrapper). The other random stuff (DoNothing, DeletePointer) can stay in bind_helpers.h, and bind.h will no longer transitively include it. Does that seem reasonable?
On 2016/10/20 21:07:51, dcheng wrote: > So I have a proposal, which I hope will make everyone happy (but is a bit more > complicated). > > We should just move Passed(), Unretained(), IgnoreResult(), etc. into bind.h. > > The support helpers for those should move into bind_internal.h (I don't think > there's anyone that's ever looked at bind_helpers.h hoping to find the > implementation of PassedWrapper). > > The other random stuff (DoNothing, DeletePointer) can stay in bind_helpers.h, > and bind.h will no longer transitively include it. > > Does that seem reasonable? So bind_helpers.h would cease to exist?
On 2016/10/20 21:10:56, robliao wrote: > On 2016/10/20 21:07:51, dcheng wrote: > > So I have a proposal, which I hope will make everyone happy (but is a bit more > > complicated). > > > > We should just move Passed(), Unretained(), IgnoreResult(), etc. into bind.h. > > > > The support helpers for those should move into bind_internal.h (I don't think > > there's anyone that's ever looked at bind_helpers.h hoping to find the > > implementation of PassedWrapper). > > > > The other random stuff (DoNothing, DeletePointer) can stay in bind_helpers.h, > > and bind.h will no longer transitively include it. > > > > Does that seem reasonable? > > So bind_helpers.h would cease to exist? Mostly, there's still some random stuff in there that doesn't really have a good home. But at least that makes it a smaller dumping ground? =)
On 2016/10/20 21:12:57, dcheng wrote: > On 2016/10/20 21:10:56, robliao wrote: > > On 2016/10/20 21:07:51, dcheng wrote: > > > So I have a proposal, which I hope will make everyone happy (but is a bit > more > > > complicated). > > > > > > We should just move Passed(), Unretained(), IgnoreResult(), etc. into > bind.h. > > > > > > The support helpers for those should move into bind_internal.h (I don't > think > > > there's anyone that's ever looked at bind_helpers.h hoping to find the > > > implementation of PassedWrapper). > > > > > > The other random stuff (DoNothing, DeletePointer) can stay in > bind_helpers.h, > > > and bind.h will no longer transitively include it. > > > > > > Does that seem reasonable? > > > > So bind_helpers.h would cease to exist? > > Mostly, there's still some random stuff in there that doesn't really have a good > home. But at least that makes it a smaller dumping ground? =) That some reasonable to me. A quick pass... Promoted to bind.h * Unretained * RetainedRef * ConstRef * Owned * Passed * IgnoreResult * Unwrap To Be Determined * DoNothing * DeletePointer * Everything else?
On 2016/10/20 21:18:12, robliao wrote: > On 2016/10/20 21:12:57, dcheng wrote: > > On 2016/10/20 21:10:56, robliao wrote: > > > On 2016/10/20 21:07:51, dcheng wrote: > > > > So I have a proposal, which I hope will make everyone happy (but is a bit > > more > > > > complicated). > > > > > > > > We should just move Passed(), Unretained(), IgnoreResult(), etc. into > > bind.h. > > > > > > > > The support helpers for those should move into bind_internal.h (I don't > > think > > > > there's anyone that's ever looked at bind_helpers.h hoping to find the > > > > implementation of PassedWrapper). > > > > > > > > The other random stuff (DoNothing, DeletePointer) can stay in > > bind_helpers.h, > > > > and bind.h will no longer transitively include it. > > > > > > > > Does that seem reasonable? > > > > > > So bind_helpers.h would cease to exist? > > > > Mostly, there's still some random stuff in there that doesn't really have a > good > > home. But at least that makes it a smaller dumping ground? =) > > That some reasonable to me. A quick pass... > > Promoted to bind.h > * Unretained > * RetainedRef > * ConstRef > * Owned > * Passed > * IgnoreResult > * Unwrap I'd say Unwrap belongs in base/bind_internal.h, along with UnretainedWrapper, RetainedRefWrapper, etc. > > To Be Determined > * DoNothing > * DeletePointer > * Everything else?
On Thu, Oct 20, 2016 at 2:18 PM, <robliao@chromium.org> wrote: > On 2016/10/20 21:12:57, dcheng wrote: > > On 2016/10/20 21:10:56, robliao wrote: > > > On 2016/10/20 21:07:51, dcheng wrote: > > > > So I have a proposal, which I hope will make everyone happy (but is > a bit > > more > > > > complicated). > > > > > > > > We should just move Passed(), Unretained(), IgnoreResult(), etc. into > > bind.h. > > > > > > > > The support helpers for those should move into bind_internal.h (I > don't > > think > > > > there's anyone that's ever looked at bind_helpers.h hoping to find > the > > > > implementation of PassedWrapper). > > > > > > > > The other random stuff (DoNothing, DeletePointer) can stay in > > bind_helpers.h, > > > > and bind.h will no longer transitively include it. > > > > > > > > Does that seem reasonable? > > > > > > So bind_helpers.h would cease to exist? > > > > Mostly, there's still some random stuff in there that doesn't really > have a > good > > home. But at least that makes it a smaller dumping ground? =) > > That some reasonable to me. A quick pass... > > Promoted to bind.h > * Unretained > * RetainedRef > * ConstRef > * Owned > * Passed > * IgnoreResult > * Unwrap > I think Unwrap is bind_internals, that's not used externally right? > > To Be Determined > * DoNothing > * DeletePointer > These two are bind_helpers IIUC > * Everything else? > Is bind_internals > > > https://codereview.chromium.org/2423423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/20 21:21:33, danakj wrote: > On Thu, Oct 20, 2016 at 2:18 PM, <mailto:robliao@chromium.org> wrote: > > > On 2016/10/20 21:12:57, dcheng wrote: > > > On 2016/10/20 21:10:56, robliao wrote: > > > > On 2016/10/20 21:07:51, dcheng wrote: > > > > > So I have a proposal, which I hope will make everyone happy (but is > > a bit > > > more > > > > > complicated). > > > > > > > > > > We should just move Passed(), Unretained(), IgnoreResult(), etc. into > > > bind.h. > > > > > > > > > > The support helpers for those should move into bind_internal.h (I > > don't > > > think > > > > > there's anyone that's ever looked at bind_helpers.h hoping to find > > the > > > > > implementation of PassedWrapper). > > > > > > > > > > The other random stuff (DoNothing, DeletePointer) can stay in > > > bind_helpers.h, > > > > > and bind.h will no longer transitively include it. > > > > > > > > > > Does that seem reasonable? > > > > > > > > So bind_helpers.h would cease to exist? > > > > > > Mostly, there's still some random stuff in there that doesn't really > > have a > > good > > > home. But at least that makes it a smaller dumping ground? =) > > > > That some reasonable to me. A quick pass... > > > > Promoted to bind.h > > * Unretained > > * RetainedRef > > * ConstRef > > * Owned > > * Passed > > * IgnoreResult > > * Unwrap > > > > I think Unwrap is bind_internals, that's not used externally right? > > > > > > To Be Determined > > * DoNothing > > * DeletePointer > > > > These two are bind_helpers IIUC > > > > * Everything else? > > > > Is bind_internals > > > > > > > https://codereview.chromium.org/2423423002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Agreed, a quick scan of Unwrap suggests it's internal to bind.
Should we close this CL?
On 2017/05/01 17:45:35, fdoray wrote: > Should we close this CL? That makes sense at this point. I'll move the action items to a bug.
Message was sent while issue was closed.
Now tracked at http://crbug.com/717138 |
