Hi all Thanks for all the responses. Since there have been quite a few separate alignments one way or another I'll try and summarize everything mentioned in one mail, being fairly liberal with the quoting and paraphrasing. Ed: please forgive. Basically it seems there are two distinct groups: Those who don't consider API changes as "functional" consider LLVM's in-tree command line tools as the "function", and consider changes to the API fair game for the NFC tag. I argued that API changes are, by definition, not NFC. It was fairly strongly rejected by several: David Blaikie:> I don't think that's how the LLVM project should/is going to classify > "functional change" - in part because there isn't a clear "public" API > boundary - there are wide interfaces across the whole project that external > users can call into.Nikita Popov:> For what it's worth, my understanding was that NFC can also include API > changes, as long as they are, well, non-functional. In LLVM pretty much > everything is part of the public API, so non-functional refactoring will > often touch the API.Mehdi Amini:> Yes I am ignoring API users, I am on the same line as Nikita here. > We don’t have stable APIs (other than the C one), so I for example I may > change an API that was taking 3 bools to take now a struct parameter > wrapping the 3 bools instead. I’ll tag it NFC.Chris Lattner mostly agrees with the above:> "NFC" is intended to talk about the behavior of the compiler, not its APIand> [...] "NFC" *encourage people* to land large changes as a series of NFC > refactorings.I think the final point is pretty telling about the use of [NFC] among llvm developers historically. Historically I can see the attraction of minimizing the cognitive barrier to addressing poorly factored code, but perhaps now it's an opportunity to rubber-stamp your commits to avoid the slow review process? When we have a mature system, fairly robust test-suite, code-review, and CI, I don't think the minor benefit of softening the armour is necessary when the sorts of changes that actually do require changing public APIs are almost never done by newcomers [citation needed]. To continue doing this when downstreams have to refactor their code every couple of days, and - crucially - find the commit to blame is frustrating. Then there are those more conservative devs whose wishes align more with my own. David Jones:> On some occasions I have dealt with API changes where compiling my old code > with the new API results in a correctly-compiling program. However, the > resulting application fails to run correctly. This issue is much harder to > track down.I think this distills my argument. Sometimes due to promotions things like this get past the compiler with no change of the user: - void maybeDoThing(unsigned Count, bool AreYouSure); + int maybeDoThing(bool AreYouSure, unsigned count); ...and then do something pretty nasty when you least expect it. If this commit is marked NFC, the committer has actively misdirected anyone who doesn't understand LLVM's unusual use of "NFC", 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.Joerg Sonnenberger:> I would consider an even more restricted subset: NFC changes would betrivial proven to be just that David Blaikie in response to Joerg and John> I disagree pretty strongly there - intent is important and all any patch > comment can describe. > > If that change was adopted, I'd want another flag for the "I intend this > not to change the observable behavior, but it's not trivially proven" - > especially when reviewing a patch that is missing test coverage. If someone > hasn't made a claim that this doesn't change behavior, then I expect that > the inverse is true (it does change behavior) and a test should be present. > And once we have that flag, I'm not sure the difference between "I intend > this to not change behavior, but it passes some threshold of triviality > that makes it not 'guaranteed'" and things below that threshold is > sufficiently valuable to encode in the commit message (& haggle over which > things are over or under that threshold).I'd say that David is already describing a tag in popular use: "NFCI" is what's conventionally used outside LLVM (and even within LLVM by others) to tag changes that match exactly that description. David also sort of foreshadowed this opinion in another message:> We could introduce a separate term but I think most NFC patches would use > that term instead, so it probably wouldn't amount to much real change - > we'd end up using that new term ubiquitously instead.Fangrui Song mentioned the following in a separate message which - to me - largely aligns with David's intent:> Sometimes people use the term "NFCI" (no functional change intended). > I thought "intended" means that: the author is not 100% sure that no > functional change is caused (for some refactoring it is sometimes > difficult to guarantee without good test coverage) > but seems that many use "NFCI" to refer to obviously NFC things.I think that in the face of an extant "NFCI" tag, these 3 opinions actually *are* compatible with each other, but whether those that advocate for "NFC" where others have described "NFCI" are aware of the existence of this tag is unclear to me. Another point was raised about downstreams and pre-commit testing by Christian Kühnel. Christian suggested integration of upstream LLVM and a number of popular downstreams' CI systems:> We could be running some *automatic integration testing with downstream > users*: Have some *public* CI machine build the heads of LLVM and a > downstream project (e.g. Rust, Tensorflow) and see if the builds still pass.I think this is noble but misses the essence of my original point: It's not that LLVM changes *per-se* that's troublesome as a downstream. It's that LLVM changes but the committer that made the change has said that it's not changed. LLVM *should* continue to change, but I still think the NFC tag is incorrect with respect to API changes, and any gentle push to make life easier to downstreams would be something I'm sure would be appreciated. Applying "[NFC]" to silent breakages goes somewhat beyond "we provide no API stability" (which is clear and well understood by the community) to "this is active misdirection that makes your life harder as a maintainer". In summary, I think there's still consensus to be made, and if us loud voices have equal standing, there's little majority, so I don't think I can formalize this for the documentation yet. Thanks for all the input so far. Apologies if I missed anyone. More voices welcome. All the Best Luke -- Codeplay Software Ltd. Company registered in England and Wales, number: 04567874 Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > Hi all > > Thanks for all the responses. > > Since there have been quite a few separate alignments one way or another I'll > try and summarize everything mentioned in one mail, being fairly liberal with > the quoting and paraphrasing. Ed: please forgive.Thanks for the summary!> Basically it seems there are two distinct groups: > > Those who don't consider API changes as "functional" consider LLVM's in-tree > command line tools as the "function", and consider changes to the API fair game > for the NFC tag. > > I argued that API changes are, by definition, not NFC. It was fairly strongly > rejected by several: > > David Blaikie: > > I don't think that's how the LLVM project should/is going to classify > > "functional change" - in part because there isn't a clear "public" API > > boundary - there are wide interfaces across the whole project that external > > users can call into. > > Nikita Popov: > > For what it's worth, my understanding was that NFC can also include API > > changes, as long as they are, well, non-functional. In LLVM pretty much > > everything is part of the public API, so non-functional refactoring will > > often touch the API. > > Mehdi Amini: > > Yes I am ignoring API users, I am on the same line as Nikita here. > > We don’t have stable APIs (other than the C one), so I for example I may > > change an API that was taking 3 bools to take now a struct parameter > > wrapping the 3 bools instead. I’ll tag it NFC. > > Chris Lattner mostly agrees with the above: > > "NFC" is intended to talk about the behavior of the compiler, not its API > > and > > > [...] "NFC" *encourage people* to land large changes as a series of NFC > > refactorings. > > I think the final point is pretty telling about the use of [NFC] among llvm > developers historically. > > Historically I can see the attraction of minimizing the cognitive barrier to > addressing poorly factored code, but perhaps now it's an opportunity to > rubber-stamp your commits to avoid the slow review process?I don't think it does that - to me the only difference with "NFC" when reviewing a patch (pre or post-commit) is "Is this intended to be testable/tested". If it says NFC I expect to see no tests updated and review the code change to ensure/verify that it shouldn't/couldn't be tested (in addition to whatever change-specific review to do).> When we have a > mature system, fairly robust test-suite, code-review, and CI, I don't think the > minor benefit of softening the armour is necessary when the sorts of changes > that actually do require changing public APIs are almost never done by newcomers > [citation needed].I lost track of this bit a bit. We do often expect newcomers to do API changes - one of the perennial "easy things for a newcomer to do" is to change APIs that take other string types to take StringRef for example - which is generally an NFC API change. (aside: folks keep saying "public API" - LLVM has no clear distinction of public (if we're using that in the sense of "outside the LLVM project") and private API - there's some library internal APIs (those where the header is in the lib directory rather than the include directory) but everything else is "inter-library API" which is about as "public" as it gets, I guess - if other parts of LLVM can use it, so can people using LLVM's libraries from outside)> To continue doing this when downstreams have to refactor their code every couple > of days, and - crucially - find the commit to blame is frustrating. > > Then there are those more conservative devs whose wishes align more with my own. > > David Jones: > > On some occasions I have dealt with API changes where compiling my old code > > with the new API results in a correctly-compiling program. However, the > > resulting application fails to run correctly. This issue is much harder to > > track down.Generally we should be somewhat careful of such a refactor because we might miss a call site inside LLVM too - but I wouldn't rule it out. It's not hard inside LLVM to search for all the call sites and update them. I think no matter the words used in the commit message I think that's always going to be a reality in the LLVM project (that there might be silently incompatible API changes).> I think this distills my argument. Sometimes due to promotions things like > this get past the compiler with no change of the user: > > - void maybeDoThing(unsigned Count, bool AreYouSure); > + int maybeDoThing(bool AreYouSure, unsigned count); > > ...and then do something pretty nasty when you least expect it. If this commit > is marked NFC, the committer has actively misdirected anyone who doesn't > understand LLVM's unusual use of "NFC",Do you have references on broader/other/"usual" uses of the term NFC and NFCI? At least my attempts to google the terms seem to only turn up LLVM and LLVM-adjacent (eg: Swift) results.> 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.> Joerg Sonnenberger: > > I would consider an even more restricted subset: NFC changes would be > trivial proven to be just that > > David Blaikie in response to Joerg and John > > I disagree pretty strongly there - intent is important and all any patch > > comment can describe. > > > > If that change was adopted, I'd want another flag for the "I intend this > > not to change the observable behavior, but it's not trivially proven" - > > especially when reviewing a patch that is missing test coverage. If someone > > hasn't made a claim that this doesn't change behavior, then I expect that > > the inverse is true (it does change behavior) and a test should be present. > > And once we have that flag, I'm not sure the difference between "I intend > > this to not change behavior, but it passes some threshold of triviality > > that makes it not 'guaranteed'" and things below that threshold is > > sufficiently valuable to encode in the commit message (& haggle over which > > things are over or under that threshold). > > I'd say that David is already describing a tag in popular use: "NFCI" is what's > conventionally used outside LLVM (and even within LLVM by others) to tag changes > that match exactly that description. > > David also sort of foreshadowed this opinion in another message: > > > We could introduce a separate term but I think most NFC patches would use > > that term instead, so it probably wouldn't amount to much real change - > > we'd end up using that new term ubiquitously instead. > > Fangrui Song mentioned the following in a separate message which - to me - > largely aligns with David's intent: > > Sometimes people use the term "NFCI" (no functional change intended). > > I thought "intended" means that: the author is not 100% sure that no > > functional change is caused (for some refactoring it is sometimes > > difficult to guarantee without good test coverage) > > but seems that many use "NFCI" to refer to obviously NFC things. > > I think that in the face of an extant "NFCI" tag, these 3 opinions actually > *are* compatible with each other, but whether those that advocate for "NFC" > where others have described "NFCI" are aware of the existence of this tag is > unclear to me. > > Another point was raised about downstreams and pre-commit testing by Christian > Kühnel. Christian suggested integration of upstream LLVM and a number of popular > downstreams' CI systems: > > We could be running some *automatic integration testing with downstream > > users*: Have some *public* CI machine build the heads of LLVM and a > > downstream project (e.g. Rust, Tensorflow) and see if the builds still pass. > > I think this is noble but misses the essence of my original point: It's not that > LLVM changes *per-se* that's troublesome as a downstream. It's that LLVM > changes but the committer that made the change has said that it's not changed. > > LLVM *should* continue to change, but I still think the NFC tag is incorrect > with respect to API changes, and any gentle push to make life easier to > downstreams would be something I'm sure would be appreciated. Applying "[NFC]" > to silent breakages goes somewhat beyond "we provide no API stability" (which > is clear and well understood by the community) to "this is active misdirection > that makes your life harder as a maintainer".Conflating the "there can be silent API breakage that causes semantic changes without build breaks" and "there are changes tagged NFC that change functionality" (due to developer mistakes/bugs) and "there are changes tagged NFC that change APIs" together is probably confusing things a bit. For instance I think the first of those is relatively unavoidable - and how something's tagged isn't going to change that. I don't think NFCI v NFC (I don't mind people choosing one or the other, but I don't think as a community we should try to split hairs and either say "you can't say NFC, you have to say NFCI always" or "here's the level of complexity over which we aren't comfortable saying it's for sure, so it's only 'intent'") really helps - all patch descriptions are statements of intent, not proven fact - we all write bugs, if we have to add defensive wording for the possibility of bugs in any commit message, I don't think that's going to improve clarity/communication. And that distinction still wouldn't address the desire for API changes to not be flagged NFC[I] - which, again, I think is a bad thing, annotating changes that don't/shouldn't be tested is a core property of the use of the NFC[I] flag. - Dave
On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi all > > Thanks for all the responses. > > Since there have been quite a few separate alignments one way or another > I'll > try and summarize everything mentioned in one mail, being fairly liberal > with > the quoting and paraphrasing. Ed: please forgive. > > > Basically it seems there are two distinct groups: > > Those who don't consider API changes as "functional" consider LLVM's > in-tree > command line tools as the "function", and consider changes to the API fair > game > for the NFC tag. > > I argued that API changes are, by definition, not NFC. It was fairly > strongly > rejected by several: > > David Blaikie: > > I don't think that's how the LLVM project should/is going to classify > > "functional change" - in part because there isn't a clear "public" API > > boundary - there are wide interfaces across the whole project that > external > > users can call into. > > Nikita Popov: > > For what it's worth, my understanding was that NFC can also include API > > changes, as long as they are, well, non-functional. In LLVM pretty much > > everything is part of the public API, so non-functional refactoring will > > often touch the API. > > Mehdi Amini: > > Yes I am ignoring API users, I am on the same line as Nikita here. > > We don’t have stable APIs (other than the C one), so I for example I may > > change an API that was taking 3 bools to take now a struct parameter > > wrapping the 3 bools instead. I’ll tag it NFC. > > Chris Lattner mostly agrees with the above: > > "NFC" is intended to talk about the behavior of the compiler, not its API > > and > > > [...] "NFC" *encourage people* to land large changes as a series of NFC > > refactorings. > > I think the final point is pretty telling about the use of [NFC] among llvm > developers historically. > > Historically I can see the attraction of minimizing the cognitive barrier > to > addressing poorly factored code, but perhaps now it's an opportunity to > rubber-stamp your commits to avoid the slow review process? When we have a > mature system, fairly robust test-suite, code-review, and CI, I don't > think the > minor benefit of softening the armour is necessary when the sorts of > changes > that actually do require changing public APIs are almost never done by > newcomers > [citation needed]. > To continue doing this when downstreams have to refactor their code every > couple > of days, and - crucially - find the commit to blame is frustrating. > > Then there are those more conservative devs whose wishes align more with > my own. > > David Jones: > > On some occasions I have dealt with API changes where compiling my old > code > > with the new API results in a correctly-compiling program. However, the > > resulting application fails to run correctly. This issue is much harder > to > > track down. > > I think this distills my argument. Sometimes due to promotions things like > this get past the compiler with no change of the user: > > - void maybeDoThing(unsigned Count, bool AreYouSure); > + int maybeDoThing(bool AreYouSure, unsigned count); > > ...and then do something pretty nasty when you least expect it. If this > commit > is marked NFC, the committer has actively misdirected anyone who doesn't > understand LLVM's unusual use of "NFC", 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. > > Joerg Sonnenberger: > > I would consider an even more restricted subset: NFC changes would be > trivial proven to be just that > > David Blaikie in response to Joerg and John > > I disagree pretty strongly there - intent is important and all any patch > > comment can describe. > > > > If that change was adopted, I'd want another flag for the "I intend this > > not to change the observable behavior, but it's not trivially proven" - > > especially when reviewing a patch that is missing test coverage. If > someone > > hasn't made a claim that this doesn't change behavior, then I expect that > > the inverse is true (it does change behavior) and a test should be > present. > > And once we have that flag, I'm not sure the difference between "I intend > > this to not change behavior, but it passes some threshold of triviality > > that makes it not 'guaranteed'" and things below that threshold is > > sufficiently valuable to encode in the commit message (& haggle over > which > > things are over or under that threshold). > > I'd say that David is already describing a tag in popular use: "NFCI" is > what's > conventionally used outside LLVM (and even within LLVM by others) to tag > changes > that match exactly that description. > > David also sort of foreshadowed this opinion in another message: > > > We could introduce a separate term but I think most NFC patches would use > > that term instead, so it probably wouldn't amount to much real change - > > we'd end up using that new term ubiquitously instead. > > Fangrui Song mentioned the following in a separate message which - to me - > largely aligns with David's intent: > > Sometimes people use the term "NFCI" (no functional change intended). > > I thought "intended" means that: the author is not 100% sure that no > > functional change is caused (for some refactoring it is sometimes > > difficult to guarantee without good test coverage) > > but seems that many use "NFCI" to refer to obviously NFC things. > > I think that in the face of an extant "NFCI" tag, these 3 opinions actually > *are* compatible with each other, but whether those that advocate for "NFC" > where others have described "NFCI" are aware of the existence of this tag > is > unclear to me. > > Another point was raised about downstreams and pre-commit testing by > Christian > Kühnel. Christian suggested integration of upstream LLVM and a number of > popular > downstreams' CI systems: > > We could be running some *automatic integration testing with downstream > > users*: Have some *public* CI machine build the heads of LLVM and a > > downstream project (e.g. Rust, Tensorflow) and see if the builds still > pass. > > I think this is noble but misses the essence of my original point: It's > not that > LLVM changes *per-se* that's troublesome as a downstream. It's that LLVM > changes but the committer that made the change has said that it's not > changed. > > LLVM *should* continue to change, but I still think the NFC tag is > incorrect > with respect to API changes, and any gentle push to make life easier to > downstreams would be something I'm sure would be appreciated. Applying > "[NFC]" > to silent breakages goes somewhat beyond "we provide no API stability" > (which > is clear and well understood by the community) to "this is active > misdirection > that makes your life harder as a maintainer". >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. 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.> In summary, I think there's still consensus to be made, and if us loud > voices > have equal standing, there's little majority, so I don't think I can > formalize > this for the documentation yet.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 :) -- Mehdi> > Thanks for all the input so far. Apologies if I missed anyone. > > More voices welcome. > > > All the Best > > Luke > -- > Codeplay Software Ltd. > Company registered in England and Wales, number: 04567874 > Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210623/345beba8/attachment.html>