[PATCH] D156192: [-Wunsafe-buffer-usage] Stop generating incorrect fix-its for variable declarations with unsupported specifiers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

LGTM!

Yeah I agree that we can eventually support a lot of these incrementally, but it's great to establish a safe baseline first.

> If there are other specifiers involved, they may be accidentally removed (e.g., fix `int [[some_type_attribute]] a[]` to `std::span<int>` a is incorrect).

Note that your patch doesn't actually address //type// attributes, only //declaration// attributes.

I'd love to learn if we handle type attributes like `int *_Nonnull` and `int *_Nonnull *_Nonnull` correctly. How do we even handle that correctly? This is no longer about preserving the attribute, it's about the very ability to put the same attribute on a `span` itself or on its template parameter. And in case of `_Nonnull` it doesn't sound like we have anything like that: there is no standard container that acts like `span` but is never null (and even if there was, that's not what `_Nonnull` really means), and we probably also can't do `span<int *_Nonnull>` because that's actually the same type as `span<int *>` (https://godbolt.org/z/oehTMbvrz).

So it's likely that we have to give up on unsupported type attributes very early, even before the fixit stage. Maybe on `hasPointerType()` stage; in such cases we aren't even sure that's a pointer, what if it's like an `int *__attribute__((vector_size(8)))`? (This actually can't happen; you can't have vectors of pointers. But something similar totally could.)

> So both `const static int * x` and `static const int * x` will be fixed to `static std::span<const int> x`.

Ugh, `const static int` sounds awful because `const`applies to the //pointee//, `static` applies to the //pointer//, then `int` applies to the //pointee// again. I hope we'll never see such code, but it's great that it works correctly!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156192/new/

https://reviews.llvm.org/D156192