[llvm-dev] "[NFC]" Abuse

David Blaikie via llvm-dev llvm-dev at lists.llvm.org
Wed Jun 23 13:31:35 PDT 2021

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:

(local to a tool, so probably not a perfect example)
(new (moved from lib/ into include/) class in
llvm/include/llvm/Transforms/IPO/Attributor.h )
(change return types and parameters from pointer to reference in
(new functions (moved from lib/ into include/) in
llvm/include/llvm/Analysis/LoopInfo.h )
(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

More information about the llvm-dev mailing list