[llvm-dev] "[NFC]" Abuse

Thu Jun 24 01:29:26 PDT 2021

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>