<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 23, 2021 at 12:37 PM John McCall via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On 23 Jun 2021, at 14:57, David Blaikie wrote:<br>
> 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:<br>
>> 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:<br>
>>> 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.<br>
><br>
> 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.<br>
<br>
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.  </blockquote><div><br></div><div>I would tend to agree, but I would also consider that the "public interfaces of a system" should be covered by testing. Which is why I go back to my litmus test: if you consider a change to not be NFC, I'd like to see a test exercising the change.</div><div><br></div><div>Since we're not doing this at all in LLVM (other than specific components, like the C APIs), I don't really consider the entirety of our C++ APIs as "public interface of the system" (they aren't tested), and so I tag (almost) all changes "NFC" when we don't expect tests changes.</div><div>Most the C++ APIs are tested through the main public LLVM API: its IR alongside with the pass entry points.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">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</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
non-private method to a class is not NFC.<br>
<br>
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.<br>
<br>
John.<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>