[llvm-dev] "[NFC]" Abuse

[David Blaikie via llvm-dev]

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