User Details
- User Since
- Apr 18 2018, 2:22 AM (256 w, 5 d)
Today
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.
Thu, Mar 16
I found one more bug.
Mon, Mar 13
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.
Thu, Mar 9
Wed, Mar 8
@NoQ I don't want to be annoying but I think you meant "warnings aren't emitted against UNEVALUATED code", is that right?
Fri, Mar 3
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.
Thu, Mar 2
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.
Wed, Mar 1
Feb 17 2023
Feb 16 2023
Feb 13 2023
Note: One thing that I have little confidence the current patch handles correctly would be qualified function names. Definitely need tests.
Feb 10 2023
Started working on conflict detection for added compatibility overload.
Feb 9 2023
We actually have only commits with warnings in release/16.x - no Fix-Its. So no need to cherry-pick.
Feb 8 2023
Feb 6 2023
LGTM
Thank you!
Feb 3 2023
Feb 2 2023
Feb 1 2023
- 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.
Jan 31 2023
Jan 30 2023
Jan 27 2023
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.
Jan 18 2023
Jan 17 2023
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
Jan 12 2023
Jan 11 2023
- made WarningGadgetSets and FixableGadgetSets own the gadgets
- changed temporary function parameter name StrategyForDirectlyUnsafeVariables to S
Jan 10 2023
Jan 9 2023
Jan 4 2023
Dec 20 2022
Thanks for the rebase!
Dec 19 2022
Dec 16 2022
LGTM
LGTM
Dec 15 2022
I did a superficial pass and the code LGTM (just nits).
Dec 9 2022
Dec 6 2022
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).
Dec 1 2022
Nov 30 2022
Nov 15 2022
@aaron.ballman Do you have any objection to us landing this?
Nov 7 2022
@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
Nov 3 2022
Thank you for the feedback Gábor!