[llvm-dev] "[NFC]" Abuse

[Luke Drummond via llvm-dev]

On Thu Jun 17, 2021 at 6:15 PM BST, David Blaikie wrote:
> Got links to the reviews? Hard to discuss in the abstract.

I was more intent on general discussion rather than singling people out, but
here these are things that have caused me to have to modify our downstream
users due to crashes or failed builds this week (these patches were not not
committed in the last week, but I'm a bit behind):
  1. [bc7d15c61da7](https://reviews.llvm.org/D101713) changed the format of the
    IR and causes crashes during inlining. Yesterday tanother user commented on
    this one on Phab saying it broke their downstream.
  2. [c83cd8feef7e](https://reviews.llvm.org/D9918l2) changed the order of
    parameters for a public function
  3. [738abfdbea2](https://github.com/llvm/llvm-project/commit/738abfdbea2)
    Apparent followup to (1). No code review linked. Reverted this morning.

> 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).

I appreciate that downstream consumers aren't afforded the same expectations of
API and ABI stability as an old-fashioned C library users, but it's not just the
function of the programs like `opt` that is the set of all things functional:
the API and the ABI are too.

> 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.
Sure. Nobody's perfect, and I'm not saying that *in-general* it's possible to
prove that *any* change - however small - doesn't affect the observable
behaviour of the program. But if we're just going to wave the NFC flag
willy-nilly it completely loses its meaning.

Since Jun 1st, there were ~130 commits in the llvm/ directory marked with
/\bNFC\b/. Many of them are simply formatting or refactoring of the bodies of
functions to clarify the codebase - and this seems appropriate usage to me. Some
of them, however, change the observable program semantics: one removes a command
line flag (breaks people's use of the llvm "program"); and one changes a public
function return type. Both these examples have the potential to break someone's
use of llvm. Yet another is a revert, which suggests to me that the use of NFC
in the original commit might have been less than judicious.

If I'm debugging a new crash, build failure, or codegen change in my downstream,
it'd be nice if I could ignore messages marked "NFC" when trying to find the
commit that introduced the new behaviour. That's *my* benchmark for
non-functional.

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