llvm dev - Apr 2016 - Refactor BitcodeWriter into classes?

> On 2016-Apr-21, at 11:41, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
> 
>> On Apr 21, 2016, at 11:25 AM, Teresa Johnson <tejohnson at
google.com> wrote:
>> 
>> I am currently making some BitcodeWriter changes that involve some
refactoring, and am thinking for the Nth time that it would be much nicer to
have a class instead of passing around a long list of parameters. I am thinking
of biting the bullet and doing that - any objections?
> 
> In general I'm worried about having single gigantic class that keep
many data members, this goes against
https://en.wikipedia.org/wiki/Single_responsibility_principle and makes it hard
to track what is initialized, where, and under which condition (basically one of
the reason why global variables are not welcome). The code is almost always
easier to understand with small separated components (yes many places are
drifting a lot in LLVM...).
> 
> Not to say that it can't be done, just that it requires a lot of care.
I don't think we should throw *all* the state in.  But some of the state
is passed everywhere.

At least (off the top of my head):

  - ValueEnumerator&
  - BitstreamWriter&

On Thu, Apr 21, 2016 at 11:44 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2016-Apr-21, at 11:41, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> >
> >
> >> On Apr 21, 2016, at 11:25 AM, Teresa Johnson <tejohnson at
google.com>
> wrote:
> >>
> >> I am currently making some BitcodeWriter changes that involve some
> refactoring, and am thinking for the Nth time that it would be much nicer
> to have a class instead of passing around a long list of parameters. I am
> thinking of biting the bullet and doing that - any objections?
> >
> > In general I'm worried about having single gigantic class that
keep many
> data members, this goes against
> https://en.wikipedia.org/wiki/Single_responsibility_principle and makes
> it hard to track what is initialized, where, and under which condition
> (basically one of the reason why global variables are not welcome). The
> code is almost always easier to understand with small separated components
> (yes many places are drifting a lot in LLVM...).
> >
> > Not to say that it can't be done, just that it requires a lot of
care.
>
> I don't think we should throw *all* the state in.  But some of the
state
> is passed everywhere.
>
> At least (off the top of my head):
>
>   - ValueEnumerator&
>   - BitstreamWriter&
>
Agree. Also the ModuleSummaryIndex, and the Module in the case of writing
bitcode for a module as opposed to a combined index (which should probably
be different derived classes). I'll try to be sensible, but you can let me
know if it is too much when I have a patch. =)

Good idea on lower-casing function names at the same time, at least for
anything that moves into the class.

Thanks,
Teresa



-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160421/2fdcee09/attachment.html>

> On Apr 21, 2016, at 11:53 AM, Teresa Johnson <tejohnson at
google.com> wrote:
> 
> 
> 
> On Thu, Apr 21, 2016 at 11:44 AM, Duncan P. N. Exon Smith <dexonsmith at
apple.com <mailto:dexonsmith at apple.com>> wrote:
> 
> > On 2016-Apr-21, at 11:41, Mehdi Amini <mehdi.amini at apple.com
<mailto:mehdi.amini at apple.com>> wrote:
> >
> >
> >> On Apr 21, 2016, at 11:25 AM, Teresa Johnson <tejohnson at
google.com <mailto:tejohnson at google.com>> wrote:
> >>
> >> I am currently making some BitcodeWriter changes that involve some
refactoring, and am thinking for the Nth time that it would be much nicer to
have a class instead of passing around a long list of parameters. I am thinking
of biting the bullet and doing that - any objections?
> >
> > In general I'm worried about having single gigantic class that
keep many data members, this goes against
https://en.wikipedia.org/wiki/Single_responsibility_principle
<https://en.wikipedia.org/wiki/Single_responsibility_principle> and makes
it hard to track what is initialized, where, and under which condition
(basically one of the reason why global variables are not welcome). The code is
almost always easier to understand with small separated components (yes many
places are drifting a lot in LLVM...).
> >
> > Not to say that it can't be done, just that it requires a lot of
care.
> 
> I don't think we should throw *all* the state in.  But some of the
state
> is passed everywhere.
> 
> At least (off the top of my head):
> 
>   - ValueEnumerator&
>   - BitstreamWriter&
> 
> Agree. Also the ModuleSummaryIndex, and the Module in the case of writing
bitcode for a module as opposed to a combined index (which should probably be
different derived classes). I'll try to be sensible, but you can let me know
if it is too much when I have a patch. =)
The problem is that most of the time, patches in isolation are perfectly fine,
it is only when you step back and see the result that you figure out you reach
the state of
https://raw.githubusercontent.com/llvm-mirror/llvm/master/lib/Target/X86/X86ISelLowering.cpp
Multiple people adding stuff after you does not help to keep the overall vision
as well.

(To be clear: I don't mean I'm opposed to what you want to do)

-- 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160421/afaeac9a/attachment.html>