[llvm-dev] "[NFC]" Abuse
David Blaikie via llvm-dev
llvm-dev at lists.llvm.org
Thu Jun 24 10:35:43 PDT 2021
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:
> This one is interesting because it’s basically just an improvement to
> the static typing in a way that should be harmless.
> 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?
> (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 )
> The “promoting existing code to public” case, sure, I can see.
> (change return types and parameters from pointer to reference in
> 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,
> (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
> "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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-dev