llvm dev - Feb 2013 - [LLVMdev] Question about changes to llvm::Argument::addAttr(AttributeSet AS) API

On 6 Feb 2013, at 07:50, Bill Wendling wrote:
> On Feb 4, 2013, at 11:54 PM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
> 
>> On 5 Feb 2013, at 01:32, Bill Wendling wrote:
>> 
>>> No. It hasn't been written up. We typically don't do
write-ups for API changes. However, we do list the thing we do change in the
ReleaseNotes (these changes haven't made it there though).
>> 
>> The attributes API has undergone a horrendous amount of churn over the
last few months, both before and after the 3.2 release.  I've lost track of
the number of times I've rewritten code interacting with this API over the
past year and I now have a mess of #ifdefs just to support 3.1, 3.2 and trunk. 
Each time, the only warning I got was that my code stopped building with trunk,
and the only way of knowing how to rewrite it was to read the attributes code.
>> 
> I'm sorry about the problems. It's a continuing problem when
working with a project where the APIs are not set in stone and are subject to
change at a moment's notice.
APIs in LLVM are not set in stone because we want to be able to evolve them as
the requirements change.  That should not be a license to constantly rewrite
APIs with large numbers of consumers, but simply an indication that if the
choice is between fixing the code and maintaining backwards compatibility then
developers are free to fix the code.  That doesn't mean that we should never
think about the cost to downstream developers when we churn APIs, it just means
that we shouldn't be held in a straitjacket by these concerns.

And the cost of changing an API is that it MUST be properly documented. 
Whenever you make the choice to break an API, you are saying 'I am saving my
time by not writing compatibility interfaces, and everyone else must rewrite
their code to support my changes.'  If you then compound this by saying
'oh, and if you want to know how to rewrite your code, then read my code, my
time is too valuable to waste documenting my work' then you do not encourage
people to depend on LLVM.
>> It would be really great if whoever is responsible could step back from
their code editor for a few minutes and write some documentation on how to
migrate from each iteration to the next.
>> 
> That would be more trouble than it's worth because the API is changing
quite rapidly right now. A lot of it hasn't solidified just yet. Basically,
once a migration doc was written it would be immediately out of date.
This comment is indicative of a code first, think second mentality.  If the APIs
are changing so quickly that even a high-level overview and broad migration
guide would be out of date before it's finished then it seems pretty clear
that the design stage was skipped entirely.  While this is fine for code on the
edges (e.g. a back-end under development, where changes don't affect any
other users), it generates a lot of problems of people when it is code that is
central to LLVM.

Effectively, here, you are saying that your time is far more valuable than
everyone else's.  You can, I hope, understand why this is not an opinion
shared by those whose time you are spending to save some of your own.  When you
save 10 minutes of your time by not writing docs, you then make every user of
this API (which means the author every out-of-tree front end, a lot of
out-of-tree passes, and some back ends) spend at least twice that time each
reading the code to try to figure out what the new equivalent of the old API is.
>> One of the goals of LLVM is to be a set of reusable libraries and this
goal is not met by gratuitous API churn with no accompanying documentation.  If
we can't have a sane deprecation strategy or an automated migration tool
then please can we at least have a token attempt at documentation?  The
AttributeSet documentation is just embarrassing.  For example, the document for
the class is:
>> 
>>> This class manages the ref count for the opaque AttributeSetImpl
>>> object and provides accessors for it
>> 
>> Great.  It's a wrapper around an opaque type that isn't
documented in the public headers.  What is an AttributeSetImpl?  What do I use
it for?  How does it relate to the last three classes that had similar
functionality but different names?
> 
> It's an opaque object. It shouldn't concern people using the
AttributeSet class. It's not documented in public headers for that very
reason.
You are missing my point.  The ONLY documentation describing AttributeSet tells
me exactly one thing: that it is a wrapper around an AttributeSetImpl.  If
AttributeSetImpl it is an opaque object that users should not need to know about
then why does the only public documentation for AttributeSet mention it at all? 
The documentation should tell me what an attribute set is (apparently, from
looking over the code, it is an ordered collection of attributes indexed by
parameter number?), not give me an implementation detail that apparently users
shouldn't need to be aware of as the sole documentation.
>> I can tell it uses the pImpl pattern by just looking at the class
definition.  That is not what the documentation should be telling me.  The rest
of this file is a case study in how not to write helpful documentation.  This
wouldn't be quite so bad if it were a class that's fairly obscure, but
this is a class that is part of the core IR that pretty much every part of the
pipeline needs to interact with.  (And that's ignoring the fact that it is
very confusing for an ordered collection that allows duplicates to be called a
set).
>> 
> Welcome to living on the top-of-tree! :-)
Living on the top of tree is unavoidable for most LLVM downstream consumers. 
Since we have no deprecation policy, you either incrementally make changes
following trunk, or you find that you need to make massive changes when a new
release is branched.  We encourage people to follow top of tree to minimise
surprises and to give feedback on evolving APIs, so saying 'well, we're
just going to churn the API's a lot, sucks to be you' to all of our
downstream consumers is a very long way from ideal.

Take a look over the list archives for the last month for the number of
questions from people using LLVM releases as old as 2.8 and you'll see what
happens when we make it hard for people to follow trunk.
> We are in between releases. It's expected that the APIs will be
unstable. I wrote (several times) that the attributes were going to be
rewritten. That implies a lot of things, including that everything about
attributes and how you use them will change. Where applicable, there will be an
auto-upgrade done. (One requirement is that the old bitcode files will continue
to be parsed correctly (at least until release 4.0).) And yes the documentation
is also in flux. It has been improving over time.
The APIs haven't just changed in trunk, they've changed with each
release.  I have some code that works with 3.1, 3.2 and trunk, and did work with
3.0 until I deleted that code to try to make it a bit cleaner.  Every single one
of these has very different APIs for accomplishing exactly the same thing.  This
is not rapid evolution to meet changing requirements, this is gratuitous API
churn caused by coding first, rather than stopping to design the APIs properly
in the first place.

Yes, it's great that you're rewriting the attributes interface now to be
stable and extensible in the longer term, but the fact that this is now the
fourth incompatible iteration of the APIs in as many releases implies that
something is badly wrong with the design process.
> The thing you quoted above is perhaps the worst comment from the
Attributes.h file, and isn't indicative of the rest of the comments in
there, which have been changing during the rewrite.
No indeed.  Most of the methods are completely lacking comments and the file
contains no explanation of the relationship between attributes, functions,
parameters, and attribute sets.
> -bw
> 
> P.S. AttributeSets don't allow duplicates. :)
Really?  In that case I'm using it wrongly.  Which I suspected anyway,
because it is almost completely undocumented.  Are the indexes not parameter
indexes?  If not, then why are they exposed at all?
> Using a development branch and then slamming those changes into trunk is at
odds with the llvm style incremental development philosophy. Living on ToT
isn't easy. No one ever said it would be. The changes that are being
complained about have only been happening for about a week now. And they're
more stable day by day.
No, they have been happening for at least a year.  If you are honestly unaware
of this, then I suggest that you stop hacking on the attributes until you're
familiar with the various iterations that those APIs have gone through over the
past two years.

David

On Feb 6, 2013, at 2:31 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
> On 6 Feb 2013, at 07:50, Bill Wendling wrote:
> 
>> On Feb 4, 2013, at 11:54 PM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
>> 
>>> On 5 Feb 2013, at 01:32, Bill Wendling wrote:
>>> 
>>>> No. It hasn't been written up. We typically don't do
write-ups for API changes. However, we do list the thing we do change in the
ReleaseNotes (these changes haven't made it there though).
>>> 
>>> The attributes API has undergone a horrendous amount of churn over
the last few months, both before and after the 3.2 release.  I've lost track
of the number of times I've rewritten code interacting with this API over
the past year and I now have a mess of #ifdefs just to support 3.1, 3.2 and
trunk.  Each time, the only warning I got was that my code stopped building with
trunk, and the only way of knowing how to rewrite it was to read the attributes
code.
>>> 
>> I'm sorry about the problems. It's a continuing problem when
working with a project where the APIs are not set in stone and are subject to
change at a moment's notice.
> 
> APIs in LLVM are not set in stone because we want to be able to evolve them
as the requirements change.  That should not be a license to constantly rewrite
APIs with large numbers of consumers, but simply an indication that if the
choice is between fixing the code and maintaining backwards compatibility then
developers are free to fix the code.  That doesn't mean that we should never
think about the cost to downstream developers when we churn APIs, it just means
that we shouldn't be held in a straitjacket by these concerns.
> 
> And the cost of changing an API is that it MUST be properly documented. 
Whenever you make the choice to break an API, you are saying 'I am saving my
time by not writing compatibility interfaces, and everyone else must rewrite
their code to support my changes.'  If you then compound this by saying
'oh, and if you want to know how to rewrite your code, then read my code, my
time is too valuable to waste documenting my work' then you do not encourage
people to depend on LLVM.
> 
You don't understand what I'm saying. The APIs were changing way too
quickly for it to make sense to create such a document. I tried as best as I
could to mitigate all of the problems, but there were several intermediate steps
that had to happen before the attributes classes were in a proper state for the
new feature work. The typical way to understand the APIs is to read the header
files and/or look at existing code. The changes I made showed how to use the new
APIs while I was going along.
>>> It would be really great if whoever is responsible could step back
from their code editor for a few minutes and write some documentation on how to
migrate from each iteration to the next.
>>> 
>> That would be more trouble than it's worth because the API is
changing quite rapidly right now. A lot of it hasn't solidified just yet.
Basically, once a migration doc was written it would be immediately out of date.
> 
> This comment is indicative of a code first, think second mentality.  If the
APIs are changing so quickly that even a high-level overview and broad migration
guide would be out of date before it's finished then it seems pretty clear
that the design stage was skipped entirely.
Not so. I had to change the existing attributes classes so that they a) still
worked while I changed them, and b) would be in a state afterwards where they
would support the new features we needed. The intermediate steps that were taken
were necessary. They would have happened if I used git or cvs or any other vcs.
> While this is fine for code on the edges (e.g. a back-end under
development, where changes don't affect any other users), it generates a lot
of problems of people when it is code that is central to LLVM.
> 
> Effectively, here, you are saying that your time is far more valuable than
everyone else's.
Wrong. I'm saying that the document would be a waste of electrons because it
would be out of date with the next change.
> You can, I hope, understand why this is not an opinion shared by those
whose time you are spending to save some of your own.  When you save 10 minutes
of your time by not writing docs, you then make every user of this API (which
means the author every out-of-tree front end, a lot of out-of-tree passes, and
some back ends) spend at least twice that time each reading the code to try to
figure out what the new equivalent of the old API is.
> 
Each step along the way had changes to existing API uses in the code base. If
you wanted examples, those were them.

On 6 Feb 2013, at 20:20, Bill Wendling wrote:
> You don't understand what I'm saying. The APIs were changing way
too quickly for it to make sense to create such a document. I tried as best as I
could to mitigate all of the problems, but there were several intermediate steps
that had to happen before the attributes classes were in a proper state for the
new feature work. The typical way to understand the APIs is to read the header
files and/or look at existing code. The changes I made showed how to use the new
APIs while I was going along.
So the AttributeSet class is going away and being replaced with something else? 
If so, then why did it even make it into the tree.  If not, then why is it not
documented.

I'm sorry, but there is no excuse for committing large changes to a core bit
of LLVM without even a brief overview of what the new class (which everyone is
now expected to use) does.  Who did the code review on this, because they really
should have objected to the complete lack of documentation?
> Not so. I had to change the existing attributes classes so that they a)
still worked while I changed them, and b) would be in a state afterwards where
they would support the new features we needed. The intermediate steps that were
taken were necessary. They would have happened if I used git or cvs or any other
vcs.
A comment saying 'this API is temporary for the migration between XXX and
YYY' takes how long to write?  Is your time really so valuable that this
extra cost is too much?

The comment on other VCSs seems irrelevant, but if you are making such invasive
changes that they must be done in multiple passes then either a feature branch
and a merge or a local git clone seem the correct ways of doing them.  Dumping
3000 lines of changes in the tree in one go is preferable to leaving the tree in
flux for a week.  You'll need the same code review in both cases, and
it's much easier for people to do this when the end result is visible than
to try to understand what all of the intermediate ones were for.
> Wrong. I'm saying that the document would be a waste of electrons
because it would be out of date with the next change.
I am starting to think that you either fundamentally misunderstand what
documentation should exist. Each of the classes should have an overview saying
what it is used for, and how it relates to other classes.  If this is going to
become wrong repeatedly, then the code should never make it to trunk, because it
implies a complete lack of any kind of design work prior to committing.

If some of the individual methods need to change, then that's fine, but they
can have a doc comment saying 'this API is currently under development and
will change over the next week'.  This saves downstream developers a lot of
time - they can just avoid updating their code until it's a bit more stable,
rather than having to .
> Each step along the way had changes to existing API uses in the code base.
If you wanted examples, those were them.
So, rather than you spending the 2 minutes required to write some quick
documentation notes, every downstream LLVM consumer has to go to the header, run
svn log to try to find the place where the old API was removed, then run svn
diff on that revision to find out what the corresponding change was in, say,
clang, and then try to map from that to their own code.

And you honestly think that this kind of behaviour is something that will
encourage people to use LLVM?

David