[llvm-dev] "[NFC]" Abuse

[Mehdi AMINI via llvm-dev]

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 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.
> _______________________________________________
> 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>