[llvm-dev] "[NFC]" Abuse

[David Blaikie via llvm-dev]

On Wed, Jun 23, 2021 at 2:57 PM John McCall <rjmccall at apple.com> wrote:

> 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.
>
And if it was removing them since they didn't end up being used anywhere,
would that not be NFC?

>
> 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.
>
Not sure I follow the comparison/equivalence you're suggesting - to me this
is NFC because it doesn't change the observable behavior of lldb (& I think
there are lots of refactors like this that get flagged NFC). Removing the
legacy pass manager currently would remove (or make no-op) various flags,
etc.

>
> 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.
>
That's not so much my feeling, I think - NFC versus non-NFC for me is
mostly an indicator of how I should review the patch. Usually hinging on
"does this have tests/is this testable".

> 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/20210624/52c594de/attachment.html>