llvm dev - Jun 2021 - "[NFC]" Abuse

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>

> if a user of an LLVM tool (clang/lld/llvm-objdump/...) can see a
difference in behaviour because of the change

As a fairly new member of the llvm community, I'm interested in what people
mean when they say "users".  For me, there are two distinct groups
that I
could consider "users" of the llvm-project: Day-to-day developers who
use
the compiled tools to build separate software, and compiler/tool developers
who integrate or build upon llvm to build a piece of software. From reading
through this thread it feels like these groups are sometimes being
conflated, which leads to disagreements regarding whether or not a change
will affect llvm "users". If we only consider group one to be
"users", then
clearly any change that does not alter the observable behaviours of the
binaries can be considered a NFC. However, if we consider group two to be
users as well, then the impacts of any change may be a functional change -
and I would personally argue that this is especially true for API changes.

As a member of group two, I have found that (as I think Luke has) changes
which are considered non-functional can sometimes have surprising effects
on our downstream components. While it is in no way the llvm project's
responsibility to avoid breaking our code, any assistance in the form of
comments or more precise tags goes a great way towards helping us to find
and diagnose places where our assumptions are inconsistent with the state
of the software.

Cheers,

-- 
*Adam Brouwers-Harries*
Compiler Engineer
aharries at upmem.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210624/eb9a74b0/attachment.html>