[llvm-dev] "[NFC]" Abuse

[John McCall via llvm-dev]

On 23 Jun 2021, at 16:31, David Blaikie 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

This one is interesting because it’s basically just an improvement to
the static typing in a way that should be harmless.

> https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253

I can see why someone could reasonably argue that this is NFC because
it’s just adding some constants in a reusable place, and that place
happens to be public.

> 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 )

The “promoting existing code to public” case, sure, I can see.

> https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979
> (change return types and parameters from pointer to reference in
> lldb/include/lldb/Breakpoint)

This is definitely beyond what I would call NFC.  It’s a good
change, but I mean, so would be not having multiple pass managers.

> 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?

Ah, no, I meant you, but I wrote it out completely wrong: I meant
that it feels like the people using NFC this broadly almost mean
that a patch *not* being NFC makes it especially questionable
and then are looking for nearly any reason to mark it NFC.

The examples you’ve given are almost all just adding API surface
area, and moreover adding it by just making something public that
wasn’t before.  To me, the purpose of NFC is to indicate that a
change is a fairly trivial and should-be-uncontroversial improvement
to the code, suitable for a quick sign-off.  So I can see an argument
those kinds of changes being NFC because the original private
interface should’ve been considered and reviewed at least a little
bit when it was added in the first place.  But no, I still don’t
think arbitrary changes to an existing API should be thought of as NFC.
That doesn’t mean they shouldn’t happen, it just means reviewers
should be at least slightly more engaged.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210623/12132698/attachment.html>