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.We could improve the doc maybe?
I consider anything modifying an external function/variable (e.g. adding a
parameter, changing the state of a default argument, deleting an unused
function, etc) a functional change.
I consider that refactoring inside a function can be NFC, e.g.
* add/delete/remove local variables
* simplify function-local code
Pure test updates can be seen NFC but I usually tag such commits as `[test]`
to make it clear no code is touched.
It may be less clear whether removing an internal linkage function /
extracting some logic into an internal linkage function is a function
change. Emmm I think that can be NFC.
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.