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):
-
bc7d15c61da7 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.
Fair point there - if a patch touches both production and test code that’s a pretty good sign it’s not NFC. I should’ve caught that in review.
-
c83cd8feef7e changed the order of
parameters for a public function
(slight typo in the URL, here’s a good one: https://reviews.llvm.org/D99182 )
LLVM doesn’t really have a concept of a “public” API - as Nikita and Mehdi have touched on - this sort of change is the bread and butter of NFC changes in LLVM - refactoring APIs (renaming, adding/removing parameters, etc) and simultaneously updating all callers in such a way that no observable change in the LLVM command line utilities can be observed.
-
738abfdbea2
Apparent followup to (1). No code review linked. Reverted this morning.
Looks like a reasonable “I thought this was NFC/that the invariant was already enforced elsewhere so the checking would be redundant here” and it didn’t turn out to be true.
This change looks in line with what Arthur should be committing with post-commit review. The opaque pointers work is going to be a lot of cleanup like this and I’m happy to review it post-commit in many cases like these small targeted changes - not sure if there’s more scope for testing that might’ve identified the reason it broke things in advance of committing the change.
Perhaps it is NFC, if (1) stuck (regardless of whether (1) was classified as NFC), I haven’t looked closely at whether it was reverted for the patch itself, or because it needed to be backed out if (1) was backed out.
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.
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.
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.
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.
I’m not proposing waving it around will-nilly, I think I (& others) have described a fairly consistent understanding of the term.
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”);
Sure, I probably would agree that shouldn’t be NFC.
and one changes a public
function return type.
I’m generally OK with that being marked NFC - as a few folks have said on this thread, cross-library API refactorings are generally understood to be “NFC” in the way the project uses the term.
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.
That the patch was reverted doesn’t necessarily mean the original commit was a problem - looks like it was reverted because a preceeding patch was reverted that meant the follow on patch needed to be reverted too, without that follow on patch necessarily being to blame.
If I’m debugging a new crash, build failure, or codegen change in my downstream,
Build failures (if you mean like Clang no longer compiling some code it did before - not “a previously compiling use of LLVM libraries now doesn’t compile anymore”) and codegen changes (if you mean LLVM produces different IR/machine code for the same source code - not LLVM itself builds to a different binary file) shouldn’t be marked NFC - that’s sort of the core of the function of LLVM and is the definition of a functional change.
Crashes - they happen.