On Thu Jun 17, 2021 at 7:15 PM BST, Mehdi AMINI wrote:> On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > > Got links to the reviews? Hard to discuss in the abstract. > > > > But generally, if it is intended to change observable behavior of the LLVM > > program (if you have to modify a lit test, that's not NFC, if one could > > craft a test (that doesn't invoke UB, though that shouldn't be possible > > through the command line interface, etc) that would need to be modified > > when the patch is committed - then it's not NFC). > > > > That's my litmus test: I see NFC is an indication that no test changes and > none are expected to be provided. The functional behavior of the compiler is > unchanged. I use NFC on API changes and refactoring when it fits this > description.I think this is a bit liberal as it ignores API users - unless I'm misunderstanding your statement about what you mean by "API changes".> We could improve the doc maybe?I'm happy to do this legwork but will hold off until something of a consensus emerges. 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 Fri, Jun 18, 2021 at 1:07 PM Luke Drummond via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Thu Jun 17, 2021 at 7:15 PM BST, Mehdi AMINI wrote: > > On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev < > > llvm-dev at lists.llvm.org> wrote: > > > > > Got links to the reviews? Hard to discuss in the abstract. > > > > > > But generally, if it is intended to change observable behavior of the > LLVM > > > program (if you have to modify a lit test, that's not NFC, if one could > > > craft a test (that doesn't invoke UB, though that shouldn't be possible > > > through the command line interface, etc) that would need to be modified > > > when the patch is committed - then it's not NFC). > > > > > > > That's my litmus test: I see NFC is an indication that no test changes > and > > none are expected to be provided. The functional behavior of the > compiler is > > unchanged. I use NFC on API changes and refactoring when it fits this > > description. > > I think this is a bit liberal as it ignores API users - unless I'm > misunderstanding your statement about what you mean by "API changes". >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. To give an example, moving some helper from Transform/Utils to Analysis would be a classical NFC change to me, even if it obviously affects the public API. Another classical NFC change is to rename a function in line with the new naming guidelines. The NFC-ness of that change should not depend on whether that function happens to be exported or not. Regards, Nikita -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210618/9d7aa5cb/attachment.html>
On Fri, Jun 18, 2021 at 4:07 AM Luke Drummond <luke.drummond at codeplay.com> wrote:> On Thu Jun 17, 2021 at 7:15 PM BST, Mehdi AMINI wrote: > > On Thu, Jun 17, 2021 at 10:15 AM David Blaikie via llvm-dev < > > llvm-dev at lists.llvm.org> wrote: > > > > > Got links to the reviews? Hard to discuss in the abstract. > > > > > > But generally, if it is intended to change observable behavior of the > LLVM > > > program (if you have to modify a lit test, that's not NFC, if one could > > > craft a test (that doesn't invoke UB, though that shouldn't be possible > > > through the command line interface, etc) that would need to be modified > > > when the patch is committed - then it's not NFC). > > > > > > > That's my litmus test: I see NFC is an indication that no test changes > and > > none are expected to be provided. The functional behavior of the > compiler is > > unchanged. I use NFC on API changes and refactoring when it fits this > > description. > > I think this is a bit liberal as it ignores API users - unless I'm > misunderstanding your statement about what you mean by "API changes".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. On the same line as my comment above, if I review a patch without any tests, I will ask if it NFC. Best, — Mehdi> > > We could improve the doc maybe? > I'm happy to do this legwork but will hold off until something of a > consensus > emerges. > > 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210618/b9c13afd/attachment.html>