On 2015/08/26 11:17:23, hanwenn wrote: > not LGTM > > I'll follow up on the ...
8 years, 7 months ago
(2015-08-26 11:22:18 UTC)
#4
On 2015/08/26 11:17:23, hanwenn wrote:
> not LGTM
>
> I'll follow up on the bug.
Well, if anybody could it would be you, working at Google. But I guess for now
you'll have to explain on the Rietveld review. Google Code has been switched to
readonly.
On 2015/08/26 11:22:18, dak wrote: > On 2015/08/26 11:17:23, hanwenn wrote: > > not LGTM ...
8 years, 7 months ago
(2015-08-26 11:26:56 UTC)
#5
On 2015/08/26 11:22:18, dak wrote:
> On 2015/08/26 11:17:23, hanwenn wrote:
> > not LGTM
> >
> > I'll follow up on the bug.
>
> Well, if anybody could it would be you, working at Google. But I guess for
now
> you'll have to explain on the Rietveld review. Google Code has been switched
to
> readonly.
"preparation of .. " : deriving from Grob is not a feature but a risk. In fact,
it would even be nice if Item and Spanner could disappear, but that seems to be
intractable.
I spent a giant amount of energy distangling formatting logic from C++ type
hierarchy, so behaviors can be mixed and matched at runtime, and this is taking
a step in the opposite direction.
On 2015/08/26 11:26:56, hanwenn wrote: > "preparation of .. " : deriving from Grob is ...
8 years, 7 months ago
(2015-08-26 21:18:49 UTC)
#6
On 2015/08/26 11:26:56, hanwenn wrote:
> "preparation of .. " : deriving from Grob is not a feature but a risk. In
fact,
> it would even be nice if Item and Spanner could disappear, but that seems to
be
> intractable.
>
> I spent a giant amount of energy distangling formatting logic from C++ type
> hierarchy, so behaviors can be mixed and matched at runtime, and this is
taking
> a step in the opposite direction.
Thanks for the feedback. David had the same concern about the rationale in the
review comment but accepted the change to the code apart from it. Will you also
be content if I limit the rationale in the commit message to the fact that
nobody currently instantiates a base Grob? Or do you object to the change on
the grounds that someone might want to instantiate a base Grob?
On 2015/08/26 21:18:49, Dan Eble wrote: > On 2015/08/26 11:26:56, hanwenn wrote: > > > ...
8 years, 7 months ago
(2015-08-28 15:17:10 UTC)
#7
On 2015/08/26 21:18:49, Dan Eble wrote:
> On 2015/08/26 11:26:56, hanwenn wrote:
>
> > "preparation of .. " : deriving from Grob is not a feature but a risk. In
> fact,
> > it would even be nice if Item and Spanner could disappear, but that seems to
> be
> > intractable.
> >
> > I spent a giant amount of energy distangling formatting logic from C++ type
> > hierarchy, so behaviors can be mixed and matched at runtime, and this is
> taking
> > a step in the opposite direction.
>
> Thanks for the feedback. David had the same concern about the rationale in
the
> review comment but accepted the change to the code apart from it. Will you
also
> be content if I limit the rationale in the commit message to the fact that
> nobody currently instantiates a base Grob?
Yes, please chnage the commit message. Also, you might want to make sure that
the rationale is properly documented.
> Or do you object to the change on
> the grounds that someone might want to instantiate a base Grob?
On 2015/08/28 15:17:10, hanwenn wrote: > On 2015/08/26 21:18:49, Dan Eble wrote: > > On ...
8 years, 7 months ago
(2015-08-28 15:18:47 UTC)
#8
On 2015/08/28 15:17:10, hanwenn wrote:
> On 2015/08/26 21:18:49, Dan Eble wrote:
> > On 2015/08/26 11:26:56, hanwenn wrote:
> >
> > > "preparation of .. " : deriving from Grob is not a feature but a risk. In
> > fact,
> > > it would even be nice if Item and Spanner could disappear, but that seems
to
> > be
> > > intractable.
> > >
> > > I spent a giant amount of energy distangling formatting logic from C++
type
> > > hierarchy, so behaviors can be mixed and matched at runtime, and this is
> > taking
> > > a step in the opposite direction.
> >
> > Thanks for the feedback. David had the same concern about the rationale in
> the
> > review comment but accepted the change to the code apart from it. Will you
> also
> > be content if I limit the rationale in the commit message to the fact that
> > nobody currently instantiates a base Grob?
>
> Yes, please chnage the commit message. Also, you might want to make sure that
> the rationale is properly documented.
(but use a different CL for that.)
On 2015/08/28 15:18:47, hanwenn wrote: > > Yes, please chnage the commit message. Also, you ...
8 years, 7 months ago
(2015-08-28 21:01:22 UTC)
#9
On 2015/08/28 15:18:47, hanwenn wrote:
> > Yes, please chnage the commit message. Also, you might want to make sure
that
> > the rationale is properly documented.
>
> (but use a different CL for that.)
I'm sorry, I don't know what you mean by CL or where specifically you want me to
document the rationale other than the commit message. Do you want it in a
comment by the declaration of Grob::clone() = 0?
Issue 260810043: Issue 4564: make Grob an abstract class
(Closed)
Created 8 years, 7 months ago by Dan Eble
Modified 8 years, 6 months ago
Reviewers: pkx166h, hanwenn, dak
Base URL:
Comments: 0