llvm dev - Apr 2016 - Refactor BitcodeWriter into classes?

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?

I assume the reason why there is no existing class wrapping the bitcode
writing process is just legacy code and nothing else?

It seems like at the least a simple class could hold the common elements
used by the various bitcode writing routines, and I might want to add a
specialized class to manage the ThinLTO combined index file generation.

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/aae2cbb0/attachment.html>

> 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.

-- 
Mehdi



 
> I assume the reason why there is no existing class wrapping the bitcode
writing process is just legacy code and nothing else?
> 
> It seems like at the least a simple class could hold the common elements
used by the various bitcode writing routines, and I might want to add a
specialized class to manage the ThinLTO combined index file generation.
> 
> Thanks,
> Teresa
> 
> -- 
> Teresa Johnson |	 Software Engineer |	 tejohnson at google.com
<mailto:tejohnson at google.com> |	 408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160421/22955465/attachment.html>

> On 2016-Apr-21, at 11:25, 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?
> 
> I assume the reason why there is no existing class wrapping the bitcode
writing process is just legacy code and nothing else?
> 
> It seems like at the least a simple class could hold the common elements
used by the various bitcode writing routines, and I might want to add a
specialized class to manage the ThinLTO combined index file generation.
Please do this.  We'll all profit.  Thanks!

Since you'll be touching all the lines anyway, it's also a good
opportunity to rename the functions to match current conventions
(i.e., start with a lower-case letter).
> 
> Thanks,
> Teresa
> 
> -- 
> Teresa Johnson |	 Software Engineer |	 tejohnson at google.com |	
408-460-2413

> 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&