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

[Bill Wendling]

On Feb 8, 2013, at 12:59 AM, David Chisnall <David.Chisnall at cl.cam.ac.uk> wrote:

> 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.
> 
Each individual method within the class was documented as best as I could make them. I missed documenting the whole class because I'm only human. It shouldn't have hindered you from using the class.

> 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.
> 
How about "FIXME: This is temporary" or "FIXME: Remove this" or putting it in the log messages? Because that's what I did.

> 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.
> 
The changes to the headers and changes to the code were in the same commits. So there's no need to track them down twice. You could have sent me a short email at the time along the lines of:

	"Bill, How the hell am I supposed to use the new classes?"

instead of wasting time crawling through several commits. In fact, I was expecting people to do just that. I would have been more than happy to help them out. But given that changing APIs is a frequent thing in LLVM, and no one else who changes APIs sends out emails to the list explaining how to update to those APIs, the thought really never occurred to me. Especially when there were examples of how to do it in the commits.

I apologize that I missed the email that you must have sent asking me how you should upgrade your code.

> 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.
> 
The attributes work was discussed quite extensively. You must have missed it. In it, it was mentioned that the current attributes classes would need to be rewritten to accommodate these changes. While I didn't specify exactly what those changes would be, it should have been a hint that things would be messy for awhile.

Again, I apologize that I missed your email where you asked what those changes would be.

> 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?
> 
Chris reviewed those changes. In fact, I implemented his vision of what the classes should look like. Since Chris was the main (read: only) person who would be reviewing said patches, I asked him for "review after commit" approval. Because of his schedule and how little time I had to work on this, he said "Yes."

You and anyone else are free to review my patches, of course.

-bw