On 23 Jun 2021, at 14:57, David Blaikie wrote:> On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> so the barrier to using LLVM becomes >> higher again. Do this enough times and using LLVM's API becomes a >> cruel and >> unusual punishment ;) >> >> John McCall: >>> Yeah, I expect NFC changes to be purely internal implementation >>> details. Changing the public interface (even to add a feature) >>> isn’t NFC; neither is anything that can affect output. > > This might be misquoted/out of context. It looks to me like John was > agreeing with my description - John's concept of "purely internal > implementation details" may include what you are describing as "Public > API" - the public interface John is probably talking about (I think, > given he opens with "yeah" in response to my description) is LLVM IR, > bitcode, and command line tools - not APIs LLVM uses internally to > implement that functionality.I may have misunderstood your point. I consider changes to the public interfaces of a system (defined as interfaces outside a library) to generally not be NFC. This is C++, so that isn’t as simple as “no changes to headers”; it means, well, roughly the sorts of things that you would describe in a C++ concept: the names and signatures of functions, the operations supported by types, and so on. Taking three bool arguments and making them a struct is not NFC. Adding a new non-private method to a class is not NFC. We should absolutely encourage refactors that improve the quality of both the implementation and its interfaces, but I don’t treat NFC as a tool to that end, and I’m surprised to hear that other maintainers do. It feels like you’re using NFC to mean almost “questionable” and then looking for any excuse not to label a patch NFC. John.
On Wed, Jun 23, 2021 at 12:37 PM John McCall <rjmccall at apple.com> wrote:> > On 23 Jun 2021, at 14:57, David Blaikie wrote: > > On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev > > <llvm-dev at lists.llvm.org> wrote: > >> so the barrier to using LLVM becomes > >> higher again. Do this enough times and using LLVM's API becomes a > >> cruel and > >> unusual punishment ;) > >> > >> John McCall: > >>> Yeah, I expect NFC changes to be purely internal implementation > >>> details. Changing the public interface (even to add a feature) > >>> isn’t NFC; neither is anything that can affect output. > > > > This might be misquoted/out of context. It looks to me like John was > > agreeing with my description - John's concept of "purely internal > > implementation details" may include what you are describing as "Public > > API" - the public interface John is probably talking about (I think, > > given he opens with "yeah" in response to my description) is LLVM IR, > > bitcode, and command line tools - not APIs LLVM uses internally to > > implement that functionality. > > I may have misunderstood your point. I consider changes to the public > interfaces of a system (defined as interfaces outside a library) to > generally not be NFC. This is C++, so that isn’t as simple as “no > changes to headers”; it means, well, roughly the sorts of things that > you would describe in a C++ concept: the names and signatures of > functions, the operations supported by types, and so on. Taking three > bool arguments and making them a struct is not NFC. Adding a new > non-private method to a class is not NFC.Ah, OK - yeah, that's not my usage/understanding of it. Here are a few examples that seem to be inconsistent with that usage, made or reviewed by fairly core/frequent LLVM contributors: https://github.com/llvm/llvm-project/commit/fdaf304e0d984c9f919c6b6b2b108d0d31cbea87 https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253 https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076 (local to a tool, so probably not a perfect example) https://github.com/llvm/llvm-project/commit/9a23e673caebdd54d8cc285fcad78f18fa2e919a (new (moved from lib/ into include/) class in llvm/include/llvm/Transforms/IPO/Attributor.h ) https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979 (change return types and parameters from pointer to reference in lldb/include/lldb/Breakpoint) https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee (new functions (moved from lib/ into include/) in llvm/include/llvm/Analysis/LoopInfo.h ) https://github.com/llvm/llvm-project/commit/84ab315574099dcac8f9fb89fe8558f8ccfbce5f (new function (moved from lib/ into include/) in Sema)> We should absolutely encourage refactors that improve the quality > of both the implementation and its interfaces, but I don’t treat > NFC as a tool to that end, and I’m surprised to hear that other > maintainers do. It feels like you’re using NFC to mean almost > “questionable” and then looking for any excuse not to label a patch > NFC."you" in this case being Luke, I take it? - Dave
On Wed, Jun 23, 2021 at 12:37 PM John McCall via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On 23 Jun 2021, at 14:57, David Blaikie wrote: > > On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev > > <llvm-dev at lists.llvm.org> wrote: > >> so the barrier to using LLVM becomes > >> higher again. Do this enough times and using LLVM's API becomes a > >> cruel and > >> unusual punishment ;) > >> > >> John McCall: > >>> Yeah, I expect NFC changes to be purely internal implementation > >>> details. Changing the public interface (even to add a feature) > >>> isn’t NFC; neither is anything that can affect output. > > > > This might be misquoted/out of context. It looks to me like John was > > agreeing with my description - John's concept of "purely internal > > implementation details" may include what you are describing as "Public > > API" - the public interface John is probably talking about (I think, > > given he opens with "yeah" in response to my description) is LLVM IR, > > bitcode, and command line tools - not APIs LLVM uses internally to > > implement that functionality. > > I may have misunderstood your point. I consider changes to the public > interfaces of a system (defined as interfaces outside a library) to > generally not be NFC.I would tend to agree, but I would also consider that the "public interfaces of a system" should be covered by testing. Which is why I go back to my litmus test: if you consider a change to not be NFC, I'd like to see a test exercising the change. Since we're not doing this at all in LLVM (other than specific components, like the C APIs), I don't really consider the entirety of our C++ APIs as "public interface of the system" (they aren't tested), and so I tag (almost) all changes "NFC" when we don't expect tests changes. Most the C++ APIs are tested through the main public LLVM API: its IR alongside with the pass entry points.> This is C++, so that isn’t as simple as “no > changes to headers”; it means, well, roughly the sorts of things that > you would describe in a C++ concept: the names and signatures of > functions, the operations supported by types, and so on. Taking three > bool arguments and making them a struct is not NFC. Adding a newnon-private method to a class is not NFC.> > We should absolutely encourage refactors that improve the quality > of both the implementation and its interfaces, but I don’t treat > NFC as a tool to that end, and I’m surprised to hear that other > maintainers do. It feels like you’re using NFC to mean almost > “questionable” and then looking for any excuse not to label a patch > NFC. > > John. > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210623/4e067ee4/attachment.html>