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

[David Chisnall]

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