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

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

On Feb 7, 2013, at 12:14 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
> 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.
> 
The AttributeSet isn't going away. I'm not sure how you got that from
what I wrote above.
> 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?
> 
If you look at the svn log messages, I did just that for temporary classes and
APIs. I also had several "FIXMEs" in the code with comments like that.
> 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.
Really? I got yelled at by Chris the last time I put several changes in the tree
within a short amount of time. It also obviates the use of the buildbots to do
something like that.
> 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 .
> 
c.f. above, re log messages and "FIXMEs".

-bw

On Feb 7, 2013, at 2:25 PM, Bill Wendling <wendling at apple.com> wrote:
> On Feb 7, 2013, at 12:14 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
> 
>> 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.
> 
> Really? I got yelled at by Chris the last time I put several changes in the
tree within a short amount of time. It also obviates the use of the buildbots to
do something like that.
> 
Oh, and the tree wasn't "in flux" in the manner you suggest. It
was stable after each commit. Everything that was temporary was mentioned as
such in the log messages and/or with FIXMEs. At any point in time, you or anyone
else could have asked me what was up (even though I wrote that there would be
sweeping changes well before they happened), and I would have gladly told you
what was stable, what you had to change to make your code work, if you should
wait, etc. Only two people did: the Mips and sanitizer people (and I suggested
that they waited, or gave them workarounds which they could use in the
meantime). I also fixed those projects I could (e.g. dragonegg). No one else
made a sound while I did this.  All of the rest I assumed didn't have
problems or knew how to fix them by looking at the patches I submitted or would
at the very least write to me and ask me how to fix their code. Again, no one
(including you) ever did this.

The only thing that *might* have been missing was a write-up of exactly what the
classes would eventually look like. However, that wouldn't have helped you
during the transition. Now that the transition is over, you can now use the
classes as you will. There may be small changes to them, but they won't be
the same sweeping changes you are complaining about here.

I worked *very* hard to make things as stable as possible as quickly as
possible. I didn't do anything "piecemeal" or
"randomly". What happened happened because I needed things to be as
stable as possible while I was changing something fundamental to LLVM.

And no, I will never "dump" 3000 lines of patches into the tree unless
they were all purely mechanical in nature. I will prefer an incremental approach
to that every time. If I did use git or whatever other vcs, I would have
submitted the patches gradually, thus achieving the same result of modifying the
tree gradually and it being "in flux" for a week.

-bw

On 7 Feb 2013, at 22:25, 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.
>> 
> The AttributeSet isn't going away. I'm not sure how you got that
from what I wrote above.
If it is not going away, then why was it committed without documentation?  The
issue is implementing new APIs that other people need to switch to, but not
documenting the new ones.  Your excuse for not documenting the new code was that
such documentation would be obsolete by the time it was committed.  I can only
assume that this meant that you were planning on removing these classes.  If
not, then the documentation would not be obsolete.

You can not simultaneously claim that documentation is too much effort to write
because the APIs change too quickly, and that undocumented APIs are stable.  If
AttributeSet is now the thing to use and will be for a reasonable amount of the
future, then it should have been committed with documentation.  If it is not,
then it should be committed with documentation saying 'WARNING: this API
will change a lot over the next few days'.  And, because this is
documentation, it belongs in the header.

I have tracked the last three rewrites of the Attributes APIs in some
out-of-tree code and in none of them have I see any documentation describing how
to migrate from the old APIs.  Most iterations, including the current one, had
nothing even approaching a high-level overview of the relevant classes.  And no,
go read the code is absolutely not documentation.  Tracking down exactly which
commit was responsible for the API change is nontrivial, and then finding all of
the related changes and trying to map them back to your own code is not an
adequate thing to recommend (as you have done twice now in this thread).  It
means that every downstream user EACH has to spend more time tracking down what
the changes are than it would take the person changing the APIs to just write a
few quick comments in a header.

I apologise if you feel that you are being singled out here, but the code that
interfaces with attributes is currently responsible for about 60% of the #ifdefs
in my code that has to work with head and the last two releases.  For something
that is effectively a bit of abstraction on a bitfield, this is a staggering
amount of API churn.  Nothing else comes close.  The second largest is
Chandler's refactoring of the header locations, and that was preceded by a
discussion on the mailing list and a detailed design and rationale.  The
attributes API changes have no public design document and no evidence that such
documentation exists: if it did then writing documentation for users would have
been trivial.

If you fixed all of the in-tree consumers of these APIs, then you must be aware
of how pervasive they are and be able to infer that they are likely to have a
similar number of out-of-tree users.  A little courtesy to these people is all
that I am requesting.  I realise that this is not entirely your fault, as the
lack of documentation should have been caught in pre-commit review, and so the
rest of us should have done a better job at that stage.  As such, I am willing
to volunteer to review the next round of changes.  Perhaps you could send me the
link to where the current set was reviewed, and let me know when you upload the
next set?

David