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 23 Jun 2021, at 16:31, David Blaikie wrote:> 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/fdaf304e0d984c9f919c6b6b2b108d0d31cbea87This one is interesting because it’s basically just an improvement to the static typing in a way that should be harmless.> https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253I can see why someone could reasonably argue that this is NFC because it’s just adding some constants in a reusable place, and that place happens to be public.> 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 )The “promoting existing code to public” case, sure, I can see.> https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979 > (change return types and parameters from pointer to reference in > lldb/include/lldb/Breakpoint)This is definitely beyond what I would call NFC. It’s a good change, but I mean, so would be not having multiple pass managers.> 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?Ah, no, I meant you, but I wrote it out completely wrong: I meant that it feels like the people using NFC this broadly almost mean that a patch *not* being NFC makes it especially questionable and then are looking for nearly any reason to mark it NFC. The examples you’ve given are almost all just adding API surface area, and moreover adding it by just making something public that wasn’t before. To me, the purpose of NFC is to indicate that a change is a fairly trivial and should-be-uncontroversial improvement to the code, suitable for a quick sign-off. So I can see an argument those kinds of changes being NFC because the original private interface should’ve been considered and reviewed at least a little bit when it was added in the first place. But no, I still don’t think arbitrary changes to an existing API should be thought of as NFC. That doesn’t mean they shouldn’t happen, it just means reviewers should be at least slightly more engaged. John. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210623/12132698/attachment.html>
I use NFC in the same sense as Dave and Mehdi (and many others) do: if a user of an LLVM tool (clang/lld/llvm-objdump/...) can see a difference in behaviour because of the change, it isn't NFC, and there should be an associated test change. Otherwise, it is an NFC change to me - pure refactors/API changes etc shouldn't impact the end user, so in my opinion can and should be marked as NFC. As someone who has helped maintain a downstream port, including in the past assisting with merging in LLVM changes, I've not had the same issues as Luke, but that's likely due to a difference in how I work in our downstream codebase. Generally, if there's an issue, I'm able to identify where the problem is likely to come from by looking at the code, and then I look for recent upstream changes that touched the area, and focus my bisecting on these (if I can't immediately find the culprit). Sometimes, this might land on an NFC change, but I don't exclude these changes, precisely because they may actually have a bug. I do however find NFC-markers useful downstream. Usually towards the end of each of our downstream release cycles, I'd review the list of upstream patches since our last release to look for any noteworthy changes that might need documenting to our end users. In these cases, I can generally skip NFC patches, since if there is a behaviour change in those patches, it wasn't likely to be a significant one, and was more likely to be a bug (and the purpose of my review isn't to find bugs). This helps speed up the process. James On Wed, 23 Jun 2021 at 21:31, David Blaikie via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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 > _______________________________________________ > 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/20210624/48abc901/attachment.html>