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

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

On Fri, Feb 8, 2013 at 12:59 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk
> wrote:
> 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.
>
I sympathize with your desire for more documentation of the end-state APIs
David, but I think you're going too far. You can argue on mailing lists
that things be done differently, but in the LLVM project I think that Bill
is within his rights to disagree about the best approach. I don't think
that it is really productive to demand so forcefully that people change
their development model to fit your suggestions. If you firmly believe that
you're method is dramatically better, I would suggest leading by example
and demonstrate how to better refactor fundamental LLVM APIs.

I also think you should remember that it is explicitly *not* a goal of the
LLVM project to optimize its development process for out-of-tree projects,
and instead it *is* a goal to optimize the development process for in-tree
efforts. I think this is a good thing, and unlikely to change. As such, I
think staging documentation updates to follow after the APIs stabilize is
not a completely unreasonable or unacceptable approach.


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'm not really disagreeing with the idea that we need documentation here...
but maybe you could help write some of the documentation since you are
impacted by these changes?


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.
>
This isn't accurate. There were in fact mailing list discussions of the
ideas behind the attribute API changes. Also, several other developers
talked directly with Bill during the rewrite, and if we had significant
concerns, would have raised them using the normal post-commit review
process.

I specifically encouraged some of the abstractions that Bill has built
because the whole point, the very core idea that was discussed on the
mailing list, is that we need attributes to represent a lot more than just
a bitfield.

> 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?
>
I don't see any need for pre-commit review here. If you see an API change
go in which could use more documentation, provide that concrete feedback in
a brief and direct email replying to the commit in question. No need for a
lengthy essay or discussion.

Even better, you could help Bill by providing patches for some
documentation, as the refactoring of the attribute APIs and representation
is an important task that no one, including you, have stepped forward to
help with...
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130208/ae561e3d/attachment.html>

On 8 Feb 2013, at 09:53, Chandler Carruth wrote:
> I also think you should remember that it is explicitly *not* a goal of the
LLVM project to optimize its development process for out-of-tree projects, and
instead it *is* a goal to optimize the development process for in-tree efforts.
>From the front page of the LLVM web site, point number 1:
> • The LLVM Core libraries provide a modern source- and target-independent
optimizer, along with code generation support for many popular CPUs (as well as
some less common ones!) These libraries are built around a well specified code
representation known as the LLVM intermediate representation ("LLVM
IR"). The LLVM Core libraries are well documented, and it is particularly
easy to invent your own language (or port an existing compiler) to use LLVM as
an optimizer and code generator.
It explicitly IS a goal (and, indeed, a very important goal as it is one of our
key differentiators from other compiler projects) for the LLVM project to
provide reusable, well-documented, libraries.

There is no mention of a distinction between in and out of tree users, and the
implication here is strongly that out of tree users are encouraged (we don't
typically accept invented languages or ports of existing compilers to use LLVM
in the tree).

I am concerned that you believe that out-of-tree consumers are unimportant. 
This has never previously been (at least publicly) a view of the LLVM community,
and our large ecosystem of out-of-tree users has traditionally been seen as an
asset.
> I think this is a good thing, and unlikely to change. As such, I think
staging documentation updates to follow after the APIs stabilize is not a
completely unreasonable or unacceptable approach.
I am concerned by the idea that documentation should come after development. 
This is a very strange workflow as it indicates that design happens after
coding.  While this is common in some projects, I would hope that LLVM would
hold itself to a higher standard.  The mindset of code-first, think-second
(document third, if at all) can be incredibly destructive to even an established
project, as we have seen in a large number of open source and proprietary
software systems.

David

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

On Feb 8, 2013, at 10:44 AM, Bill Wendling <wendling at apple.com> wrote:
> 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."
I just want to clarify that, while Chris came up with the initial design, there
was a lot of input from people in the community on it as well. I didn't mean
to leave anyone out. :-)

-bw