llvm dev - Jun 2021 - "[NFC]" Abuse

On 17 Jun 2021, at 13:15, David Blaikie via llvm-dev 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).
>
> But I think it's important that NFC is about intent, not about the
reality
> of what the patch ends up doing - so we can't judge an NFC patch by
whether
> it causes a regression later - the NFC marker is there to say "I
don't
> intend this to cause any observable change, if you observe any change,
> that's a bug" in the same way any patch description is a statement
of
> intent and can't be more than that.
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.

There are a few places where LLVM does track global state that we don’t
normally consider “output”.  I think a change that created fewer
llvm::ConstantExpr’s by, say, not generating a bitcast that only gets
stripped/replaced a few cycles later would still count as NFC to me,
since we don’t really consider the set of unused ConstantExprs to
be output.  But those are clarifications of the general rule, not
exceptions.

John.

On Thu Jun 17, 2021 at 8:13 PM BST, John McCall wrote:
> 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.
>
> There are a few places where LLVM does track global state that we
> don’t
> normally consider “output”. I think a change that created fewer
> llvm::ConstantExpr’s by, say, not generating a bitcast that only gets
> stripped/replaced a few cycles later would still count as NFC to me,
> since we don’t really consider the set of unused ConstantExprs to
> be output. But those are clarifications of the general rule, not
> exceptions.
>
This seems like a sensible balance to me.
> John.
-- 
Codeplay Software Ltd.
Company registered in England and Wales, number: 04567874
Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF

On Thu, Jun 17, 2021 at 03:13:16PM -0400, John McCall via llvm-dev
wrote:
> 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.
I would consider an even more restricted subset: NFC changes would be
trivial proven to be just that. That means non-trivial algorithmic
changes are certainly not NFC. My point is that the tag should be
applied with care only and if there is any doubt, it should not be used
at all.

Joerg