<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/xhtml; charset=utf-8">
</head>
<body>
<div style="font-family:sans-serif"><div style="white-space:normal">
<p dir="auto">On 23 Jun 2021, at 16:31, David Blaikie wrote:</p>
</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #3983C4; color:#3983C4; margin:0 0 5px; padding-left:5px"><p dir="auto">On Wed, Jun 23, 2021 at 12:37 PM John McCall <rjmccall@apple.com> wrote:</p>
<blockquote style="border-left:2px solid #3983C4; color:#7CBF0C; margin:0 0 5px; padding-left:5px; border-left-color:#7CBF0C"><p dir="auto">On 23 Jun 2021, at 14:57, David Blaikie wrote:</p>
<blockquote style="border-left:2px solid #3983C4; color:#7CBF0C; margin:0 0 5px; padding-left:5px; border-left-color:#7CBF0C"><p dir="auto">On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev<br>
<llvm-dev@lists.llvm.org> wrote:</p>
<blockquote style="border-left:2px solid #3983C4; color:#7CBF0C; margin:0 0 5px; padding-left:5px; border-left-color:#7CBF0C"><p dir="auto">so the barrier to using LLVM becomes<br>
higher again. Do this enough times and using LLVM's API becomes a<br>
cruel and<br>
unusual punishment ;)<br>
<br>
John McCall:</p>
<blockquote style="border-left:2px solid #3983C4; color:#7CBF0C; margin:0 0 5px; padding-left:5px; border-left-color:#7CBF0C"><p dir="auto">Yeah, I expect NFC changes to be purely internal implementation<br>
details. Changing the public interface (even to add a feature)<br>
isn’t NFC; neither is anything that can affect output.</p>
</blockquote></blockquote><p dir="auto">This might be misquoted/out of context. It looks to me like John was<br>
agreeing with my description - John's concept of "purely internal<br>
implementation details" may include what you are describing as "Public<br>
API" - the public interface John is probably talking about (I think,<br>
given he opens with "yeah" in response to my description) is LLVM IR,<br>
bitcode, and command line tools - not APIs LLVM uses internally to<br>
implement that functionality.</p>
</blockquote><p dir="auto">I may have misunderstood your point. I consider changes to the public<br>
interfaces of a system (defined as interfaces outside a library) to<br>
generally not be NFC. This is C++, so that isn’t as simple as “no<br>
changes to headers”; it means, well, roughly the sorts of things that<br>
you would describe in a C++ concept: the names and signatures of<br>
functions, the operations supported by types, and so on. Taking three<br>
bool arguments and making them a struct is not NFC. Adding a new<br>
non-private method to a class is not NFC.</p>
</blockquote><p dir="auto">Ah, OK - yeah, that's not my usage/understanding of it.<br>
<br>
Here are a few examples that seem to be inconsistent with that usage,<br>
made or reviewed by fairly core/frequent LLVM contributors:<br>
<br>
<a href="https://github.com/llvm/llvm-project/commit/fdaf304e0d984c9f919c6b6b2b108d0d31cbea87">https://github.com/llvm/llvm-project/commit/fdaf304e0d984c9f919c6b6b2b108d0d31cbea87</a></p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">This one is interesting because it’s basically just an improvement to<br>
the static typing in a way that should be harmless.</p>
</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #3983C4; color:#3983C4; margin:0 0 5px; padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253">https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253</a></p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">I can see why someone could reasonably argue that this is NFC because<br>
it’s just adding some constants in a reusable place, and that place<br>
happens to be public.</p>
</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #3983C4; color:#3983C4; margin:0 0 5px; padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076">https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076</a><br>
(local to a tool, so probably not a perfect example)<br>
<a href="https://github.com/llvm/llvm-project/commit/9a23e673caebdd54d8cc285fcad78f18fa2e919a">https://github.com/llvm/llvm-project/commit/9a23e673caebdd54d8cc285fcad78f18fa2e919a</a><br>
(new (moved from lib/ into include/) class in<br>
llvm/include/llvm/Transforms/IPO/Attributor.h )</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">The “promoting existing code to public” case, sure, I can see.</p>
</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #3983C4; color:#3983C4; margin:0 0 5px; padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979">https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979</a><br>
(change return types and parameters from pointer to reference in<br>
lldb/include/lldb/Breakpoint)</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">This is definitely beyond what I would call NFC. It’s a good<br>
change, but I mean, so would be not having multiple pass managers.</p>
</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #3983C4; color:#3983C4; margin:0 0 5px; padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee">https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee</a><br>
(new functions (moved from lib/ into include/) in<br>
llvm/include/llvm/Analysis/LoopInfo.h )<br>
<a href="https://github.com/llvm/llvm-project/commit/84ab315574099dcac8f9fb89fe8558f8ccfbce5f">https://github.com/llvm/llvm-project/commit/84ab315574099dcac8f9fb89fe8558f8ccfbce5f</a><br>
(new function (moved from lib/ into include/) in Sema)<br>
</p>
<blockquote style="border-left:2px solid #3983C4; color:#7CBF0C; margin:0 0 5px; padding-left:5px; border-left-color:#7CBF0C"><p dir="auto">We should absolutely encourage refactors that improve the quality<br>
of both the implementation and its interfaces, but I don’t treat<br>
NFC as a tool to that end, and I’m surprised to hear that other<br>
maintainers do. It feels like you’re using NFC to mean almost<br>
“questionable” and then looking for any excuse not to label a patch<br>
NFC.</p>
</blockquote><p dir="auto">"you" in this case being Luke, I take it?</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">Ah, no, I meant you, but I wrote it out completely wrong: I meant<br>
that it feels like the people using NFC this broadly almost mean<br>
that a patch <em>not</em> being NFC makes it especially questionable<br>
and then are looking for nearly any reason to mark it NFC.</p>
<p dir="auto">The examples you’ve given are almost all just adding API surface<br>
area, and moreover adding it by just making something public that<br>
wasn’t before. To me, the purpose of NFC is to indicate that a<br>
change is a fairly trivial and should-be-uncontroversial improvement<br>
to the code, suitable for a quick sign-off. So I can see an argument<br>
those kinds of changes being NFC because the original private<br>
interface should’ve been considered and reviewed at least a little<br>
bit when it was added in the first place. But no, I still don’t<br>
think arbitrary changes to an existing API should be thought of as NFC.<br>
That doesn’t mean they shouldn’t happen, it just means reviewers<br>
should be at least slightly more engaged.</p>
<p dir="auto">John.</p>
</div>
</div>
</body>
</html>