<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 10:34 AM Luke Drummond via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">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">Hi all<br>
<br>
Thanks for all the responses.<br>
<br>
Since there have been quite a few separate alignments one way or another I'll<br>
try and summarize everything mentioned in one mail, being fairly liberal with<br>
the quoting and paraphrasing. Ed: please forgive.<br>
<br>
<br>
Basically it seems there are two distinct groups:<br>
<br>
Those who don't consider API changes as "functional" consider LLVM's in-tree<br>
command line tools as the "function", and consider changes to the API fair game<br>
for the NFC tag.<br>
<br>
I argued that API changes are, by definition, not NFC. It was fairly strongly<br>
rejected by several:<br>
<br>
David Blaikie:<br>
> I don't think that's how the LLVM project should/is going to classify<br>
> "functional change" - in part because there isn't a clear "public" API<br>
> boundary - there are wide interfaces across the whole project that external<br>
> users can call into.<br>
<br>
Nikita Popov:<br>
> For what it's worth, my understanding was that NFC can also include API<br>
> changes, as long as they are, well, non-functional. In LLVM pretty much<br>
> everything is part of the public API, so non-functional refactoring will<br>
> often touch the API.<br>
<br>
Mehdi Amini:<br>
> Yes I am ignoring API users, I am on the same line as Nikita here.<br>
> We don’t have stable APIs (other than the C one), so I for example I may<br>
> change an API that was taking 3 bools to take now a struct parameter<br>
> wrapping the 3 bools instead. I’ll tag it NFC.<br>
<br>
Chris Lattner mostly agrees with the above:<br>
> "NFC" is intended to talk about the behavior of the compiler, not its API<br>
<br>
and<br>
<br>
> [...] "NFC" *encourage people* to land large changes as a series of NFC<br>
> refactorings.<br>
<br>
I think the final point is pretty telling about the use of [NFC] among llvm<br>
developers historically.<br>
<br>
Historically I can see the attraction of minimizing the cognitive barrier to<br>
addressing poorly factored code, but perhaps now it's an opportunity to<br>
rubber-stamp your commits to avoid the slow review process? When we have a<br>
mature system, fairly robust test-suite, code-review, and CI, I don't think the<br>
minor benefit of softening the armour is necessary when the sorts of changes<br>
that actually do require changing public APIs are almost never done by newcomers<br>
[citation needed].<br>
To continue doing this when downstreams have to refactor their code every couple<br>
of days, and - crucially - find the commit to blame is frustrating.<br>
<br>
Then there are those more conservative devs whose wishes align more with my own.<br>
<br>
David Jones:<br>
> On some occasions I have dealt with API changes where compiling my old code<br>
> with the new API results in a correctly-compiling program. However, the<br>
> resulting application fails to run correctly. This issue is much harder to<br>
> track down.<br>
<br>
I think this distills my argument. Sometimes due to promotions things like<br>
this get past the compiler with no change of the user:<br>
<br>
- void maybeDoThing(unsigned Count, bool AreYouSure);<br>
+ int maybeDoThing(bool AreYouSure, unsigned count);<br>
<br>
...and then do something pretty nasty when you least expect it. If this commit<br>
is marked NFC, the committer has actively misdirected anyone who doesn't<br>
understand LLVM's unusual use of "NFC", so the barrier to using LLVM becomes<br>
higher again. Do this enough times and using LLVM's API becomes a 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>
Joerg Sonnenberger:<br>
> I would consider an even more restricted subset: NFC changes would be<br>
trivial proven to be just that<br>
<br>
David Blaikie in response to Joerg and John<br>
> I disagree pretty strongly there - intent is important and all any patch<br>
> comment can describe.<br>
><br>
> If that change was adopted, I'd want another flag for the "I intend this<br>
> not to change the observable behavior, but it's not trivially proven" -<br>
> especially when reviewing a patch that is missing test coverage. If someone<br>
> hasn't made a claim that this doesn't change behavior, then I expect that<br>
> the inverse is true (it does change behavior) and a test should be present.<br>
> And once we have that flag, I'm not sure the difference between "I intend<br>
> this to not change behavior, but it passes some threshold of triviality<br>
> that makes it not 'guaranteed'" and things below that threshold is<br>
> sufficiently valuable to encode in the commit message (& haggle over which<br>
> things are over or under that threshold).<br>
<br>
I'd say that David is already describing a tag in popular use: "NFCI" is what's<br>
conventionally used outside LLVM (and even within LLVM by others) to tag changes<br>
that match exactly that description.<br>
<br>
David also sort of foreshadowed this opinion in another message:<br>
<br>
> We could introduce a separate term but I think most NFC patches would use<br>
> that term instead, so it probably wouldn't amount to much real change -<br>
> we'd end up using that new term ubiquitously instead.<br>
<br>
Fangrui Song mentioned the following in a separate message which - to me -<br>
largely aligns with David's intent:<br>
> Sometimes people use the term "NFCI" (no functional change intended).<br>
> I thought "intended" means that: the author is not 100% sure that no<br>
> functional change is caused (for some refactoring it is sometimes<br>
> difficult to guarantee without good test coverage)<br>
> but seems that many use "NFCI" to refer to obviously NFC things.<br>
<br>
I think that in the face of an extant "NFCI" tag, these 3 opinions actually<br>
*are* compatible with each other, but whether those that advocate for "NFC"<br>
where others have described "NFCI" are aware of the existence of this tag is<br>
unclear to me.<br>
<br>
Another point was raised about downstreams and pre-commit testing by Christian<br>
Kühnel. Christian suggested integration of upstream LLVM and a number of popular<br>
downstreams' CI systems:<br>
>  We could be running some *automatic integration testing with downstream<br>
>  users*: Have some *public* CI machine build the heads of LLVM and a<br>
>  downstream project (e.g. Rust, Tensorflow) and see if the builds still pass.<br>
<br>
I think this is noble but misses the essence of my original point: It's not that<br>
LLVM changes *per-se* that's troublesome as a downstream. It's that LLVM<br>
changes but the committer that made the change has said that it's not changed.<br>
<br>
LLVM *should* continue to change, but I still think the NFC tag is incorrect<br>
with respect to API changes, and any gentle push to make life easier to<br>
downstreams would be something I'm sure would be appreciated. Applying "[NFC]"<br>
to silent breakages goes somewhat beyond "we provide no API stability" (which<br>
is clear and well understood by the community) to "this is active misdirection<br>
that makes your life harder as a maintainer".<br></blockquote><div><br></div><div>I suspect there is an irreconcilable underlying motivation here in how we're looking at it, in terms of who is the intended target of this signaling. I feel like you're looking at getting a signal through NFC for a downstream project looking at finding issues during their integration, while I'm not using NFC for this purpose. Instead I'm using NFC as a signaling tool internal to the project development itself.</div><div> it is a signal for reviewers of my revision (no test change here, and also "Please reviewer: validate my intent here, am I really not changing the behavior of the compiler?") and vice-versa when I am the reviewer. It is also a potential signal for narrowing down a blamelist sent by broken bot post-commit (I've also used it to pre-filter a list of commits when tracking performance regressions in the past). It isn't bulletproof: a `git bisect` can lead to a NFC commit, but that's because we're human and we make mistakes.</div><div><br></div><div><br></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">
In summary, I think there's still consensus to be made, and if us loud voices<br>
have equal standing, there's little majority, so I don't think I can formalize<br>
this for the documentation yet. </blockquote><div><br></div><div>There are two things here, "should we change what we do to be more strict", on which I don't see a consensus here, and "should we document better how this tag has been widely used till now", which is a different thing :) </div><div><br></div><div>-- </div><div>Mehdi</div><div><br></div><div><br></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">
<br>
Thanks for all the input so far. Apologies if I missed anyone.<br>
<br>
More voices welcome.<br>
<br>
<br>
All the Best<br>
<br>
Luke<br>
-- <br>
Codeplay Software Ltd.<br>
Company registered in England and Wales, number: 04567874<br>
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF<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>