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

[David Chisnall]

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