Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1932)

Issue 549890043: Make GET_LISTENER take a pointer arg and deduce its type

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 11 months ago by dak
Modified:
3 years, 11 months ago
Reviewers:
Dan Eble, lemzwerg, hanwenn
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Make GET_LISTENER take a pointer arg and deduce its type Instead of Context *p; [...] Listener l = p->GET_LISTENER (Context, somecallback); this now uses Context *p; [...] Listener l = GET_LISTENER (p, somecallback); This removes the necessity for providing the somewhat awkward type argument.

Patch Set 1 #

Patch Set 2 : formatting #

Total comments: 1

Patch Set 3 : Remove ties of listener.hh to smobs.hh, reducing dependencies #

Total comments: 4

Patch Set 4 : Remove/move obviated includes of listener.hh #

Patch Set 5 : refactor a bit, use type_traits unconditionally #

Patch Set 6 : Use C++ magic to derive proper member function pointer base class #

Patch Set 7 : Create and use type_traitor mfp_baseclass #

Patch Set 8 : Cleanup. Use std::remove_pointer rather than std:remove_reference. Doh. #

Patch Set 9 : Use the support from issue 5920 in callback.hh (included for reference but not actually changed by … #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -68 lines) Patch
M lily/context.cc View 1 2 3 4 chunks +8 lines, -7 lines 0 comments Download
M lily/dispatcher.cc View 4 chunks +4 lines, -4 lines 0 comments Download
M lily/engraver-group.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M lily/global-context.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M lily/grace-engraver.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M lily/include/callback.hh View 1 2 3 4 5 6 7 8 1 chunk +50 lines, -0 lines 0 comments Download
M lily/include/context.hh View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M lily/include/listener.hh View 1 2 3 4 5 6 7 8 2 chunks +26 lines, -15 lines 0 comments Download
M lily/include/paper-column-engraver.hh View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M lily/include/smobs.hh View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M lily/include/translator-group.hh View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M lily/lyric-combine-music-iterator.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M lily/midi-cc-performer.cc View 1 2 chunks +2 lines, -4 lines 0 comments Download
M lily/score-engraver.cc View 2 chunks +6 lines, -6 lines 0 comments Download
M lily/score-performer.cc View 1 chunk +6 lines, -6 lines 0 comments Download
M lily/smobs.cc View 1 2 1 chunk +0 lines, -7 lines 0 comments Download
M lily/translator.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M lily/translator-group.cc View 1 2 3 3 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 20
dak
formatting
3 years, 11 months ago (2020-04-18 00:04:41 UTC) #1
lemzwerg
LGTM - but I don't know the internals well enough to really decide that. What ...
3 years, 11 months ago (2020-04-18 05:40:53 UTC) #2
dak
Remove ties of listener.hh to smobs.hh, reducing dependencies
3 years, 11 months ago (2020-04-18 10:18:07 UTC) #3
dak
On 2020/04/18 05:40:53, lemzwerg wrote: > LGTM - but I don't know the internals well ...
3 years, 11 months ago (2020-04-18 10:26:01 UTC) #4
hanwenn
https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh File lily/include/listener.hh (left): https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh#oldcode140 lily/include/listener.hh:140: #define GET_LISTENER(cl, proc) get_listener (Callback_wrapper::make_smob<Listener::trampoline<cl, &cl::proc> > ()) IMO, ...
3 years, 11 months ago (2020-04-18 10:30:27 UTC) #5
lemzwerg
> I agree with the sentiment, but I fail to come up > with a ...
3 years, 11 months ago (2020-04-18 10:32:50 UTC) #6
dak
On 2020/04/18 10:26:01, dak wrote: > On 2020/04/18 05:40:53, lemzwerg wrote: > > > What ...
3 years, 11 months ago (2020-04-18 10:43:47 UTC) #7
dak
On 2020/04/18 10:32:50, lemzwerg wrote: > > I agree with the sentiment, but I fail ...
3 years, 11 months ago (2020-04-18 10:45:44 UTC) #8
dak
On 2020/04/18 10:30:27, hanwenn wrote: > https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh > File lily/include/listener.hh (left): > > https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh#oldcode140 > ...
3 years, 11 months ago (2020-04-18 10:55:32 UTC) #9
dak
Remove/move obviated includes of listener.hh
3 years, 11 months ago (2020-04-18 11:47:53 UTC) #10
Dan Eble
LGTM but there are a couple of things I strongly encourage considering. https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc File lily/global-context.cc ...
3 years, 11 months ago (2020-04-18 13:54:30 UTC) #11
dak
https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc File lily/global-context.cc (right): https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc#newcode49 lily/global-context.cc:49: event_source ()->add_listener (GET_LISTENER (static_cast<Context *> (this), create_context_from_event), On 2020/04/18 ...
3 years, 11 months ago (2020-04-18 14:31:33 UTC) #12
Dan Eble
On 2020/04/18 14:31:33, dak wrote: > If we consider type_traits as a sunk cost, I'll ...
3 years, 11 months ago (2020-04-18 14:43:24 UTC) #13
dak
refactor a bit, use type_traits unconditionally
3 years, 11 months ago (2020-04-18 17:33:32 UTC) #14
dak
Use C++ magic to derive proper member function pointer base class
3 years, 11 months ago (2020-04-18 17:34:29 UTC) #15
dak
On 2020/04/18 14:43:24, Dan Eble wrote: > On 2020/04/18 14:31:33, dak wrote: > > If ...
3 years, 11 months ago (2020-04-18 17:53:34 UTC) #16
Dan Eble
On 2020/04/18 17:53:34, dak wrote: > Well, after a refactoring (patch set 5) it was ...
3 years, 11 months ago (2020-04-18 18:16:05 UTC) #17
dak
Create and use type_traitor mfp_baseclass
3 years, 11 months ago (2020-04-18 18:19:56 UTC) #18
dak
Cleanup. Use std::remove_pointer rather than std:remove_reference. Doh.
3 years, 11 months ago (2020-04-18 19:49:10 UTC) #19
dak
3 years, 11 months ago (2020-04-20 19:39:23 UTC) #20
Use the support from issue 5920 in callback.hh (included for reference but not
actually changed by this issue any more)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b