<div dir="ltr"><div dir="ltr">On Wed, Jun 23, 2021 at 2:57 PM John McCall <<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>




<div>
<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 rgb(57,131,196);color:rgb(57,131,196);margin:0px 0px 5px;padding-left:5px"><p dir="auto">On Wed, Jun 23, 2021 at 12:37 PM John McCall <<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>> wrote:</p>
<blockquote style="border-left:2px solid rgb(124,191,12);color:rgb(124,191,12);margin:0px 0px 5px;padding-left:5px"><p dir="auto">On 23 Jun 2021, at 14:57, David Blaikie wrote:</p>
<blockquote style="border-left:2px solid rgb(124,191,12);color:rgb(124,191,12);margin:0px 0px 5px;padding-left:5px"><p dir="auto">On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:</p>
<blockquote style="border-left:2px solid rgb(124,191,12);color:rgb(124,191,12);margin:0px 0px 5px;padding-left:5px"><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 rgb(124,191,12);color:rgb(124,191,12);margin:0px 0px 5px;padding-left:5px"><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" target="_blank">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 rgb(57,131,196);color:rgb(57,131,196);margin:0px 0px 5px;padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/56709b869570f7825d335d633bc829511980c253" target="_blank">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></div></blockquote><div>And if it was removing them since they didn't end up being used anywhere, would that not be NFC? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div style="font-family:sans-serif"><div style="white-space:normal">

</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid rgb(57,131,196);color:rgb(57,131,196);margin:0px 0px 5px;padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/dd1b121c99de6bd7186e23e2bf34edb02db7c076" target="_blank">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" target="_blank">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 rgb(57,131,196);color:rgb(57,131,196);margin:0px 0px 5px;padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/cfb96d845a684a5c567823dbe2aa4392937ee979" target="_blank">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></div></blockquote><div>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.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div style="font-family:sans-serif"><div style="white-space:normal">

</div>
<div style="white-space:normal"><blockquote style="border-left:2px solid rgb(57,131,196);color:rgb(57,131,196);margin:0px 0px 5px;padding-left:5px"><p dir="auto"><a href="https://github.com/llvm/llvm-project/commit/7629b2a09c169bfd7f7295deb3678f3fa7755eee" target="_blank">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" target="_blank">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 rgb(124,191,12);color:rgb(124,191,12);margin:0px 0px 5px;padding-left:5px"><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></div></div></div></blockquote><div>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".  <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div style="font-family:sans-serif"><div style="white-space:normal">

<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>
</div>

</blockquote></div></div>