LGTM

Note: The naive strategy doesn't add #include <span> to header files declaring a function which parameter we change.
We might have to track all the files we add a reference to a new type and make sure all of those have the necessary headers included.

I found one more bug.

I just discovered my original prototype had yet another bug - parameters that use "array syntax" would get spanified but their name would be missing.

+1 to moving the comparator to the .cpp file; otherwise LGTM!
Thanks for fixing this!

We agreed with @ymandel to have a separate discussion about how can Safe Buffers Fix-Its benefit from using libClangTransformers .
Our tentative plan is to it for new code that we write and eventually change all our code to use it.

@NoQ I don't want to be annoying but I think you meant "warnings aren't emitted against UNEVALUATED code", is that right?

This is an interesting topic. In the abstract I see the question as should the Fix-Its prioritize how the code will fit the desired end state (presumably modern idiomatic C++) or carefully respect the state of the code as is now.

As we discussed offline - we should add tests that our overload conflict detection would notice overloads that are declared below the function that's being analyzed.
Since our analysis callback is invoked after a body function has been parsed (but before the whole TU is parsed) it is not immediately clear if all function declarations in the TU will already be represented in the AST at that point.

Note: One thing that I have little confidence the current patch handles correctly would be qualified function names. Definitely need tests.

Started working on conflict detection for added compatibility overload.

We actually have only commits with warnings in release/16.x - no Fix-Its. So no need to cherry-pick.

LGTM
Thank you!

• fixed all crashes in our tests
• added cases of functions where we should not emit Fix-Its for parameters

rebased

rebased

• rebased
• moved compatibility decl/def below original

I got a test failure in SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp which I believe is caused solely by the fact that we emit the diagnostics in different order.
I am not sure it matters and since the Fix-Its clearly specify what source lines they apply to I suggest we simply replace every // CHECK: with // CHECK-DAG:.
That fixed the problem for me.

I think that my current name of the Fixable is pretty bad - open to suggestions!

I am sorry I haven't notice this earlier - let's fix this before we land the patch.

The patch has already landed in:
https://github.com/llvm/llvm-project/commit/fe93da22aa7bd57e277571cd692c7c0cc51c0478

Respect 80-char line limit in changes to DiagnosticSemaKinds.td.

Addressed the comments about use of auto in declarations.

LGTM

Deprecate in favor of https://reviews.llvm.org/D141725

• made WarningGadgetSets and FixableGadgetSets own the gadgets
• changed temporary function parameter name StrategyForDirectlyUnsafeVariables to S

In D139737#4042093, @NoQ wrote:

It sounds to me as if by landing the patch we'll temporarily make the compiler emit incorrect fixes. I think we should avoid doing that. Is it possible to wait until our first proof-of-concept FixableGadget lands before landing this patch? I think there shouldn't be too much conflict between such patches.

Thanks for the rebase!

LGTM

LGTM

I did a superficial pass and the code LGTM (just nits).

Hi @erichkeane,
I am sorry to bother you but in case you are ok with the patch could you please approve?
It sounds like you are taking a break from reviews starting later this week until January.
I'd appreciate if you could unblock us (unless you still have some comments).

@aaron.ballman Do you have any objection to us landing this?

@aaron.ballman We'd like to start making progress on the implementation in parallel to iterating on the documentation and this is our first patch:
https://reviews.llvm.org/D137346

Thank you for the feedback Gábor!