Skip to content

Comments

Interpret gsl::Pointer as making types unsafe (gated behind a flag).#505

Open
copybara-service[bot] wants to merge 1 commit intomainfrom
test_872978169
Open

Interpret gsl::Pointer as making types unsafe (gated behind a flag).#505
copybara-service[bot] wants to merge 1 commit intomainfrom
test_872978169

Conversation

@copybara-service
Copy link

Interpret gsl::Pointer as making types unsafe (gated behind a flag).

See the docs: https://clang.llvm.org/docs/AttributeReference.html#pointer

The attribute [[gsl::Pointer(T)]] applies to structs and classes that behave like pointers to an object of type T

In other words, this is meant to capture the concept that it might even dangle. Example types that have a Pointer annotation include:

  • string_view
  • std::span
  • absl::Span
  • various non-pointer iterator types
  • FunctionRef
  • ...

Very clearly, we want all of these to be unsafe. While the exact meaning of gsl::Pointer is subject to change in clang, I think we can reliably use it as signal that a type should be an unsafe type.

Of course, this change is backwards-incompatible, so it is gated behind a flag, but we can turn it on atomically once this is released to crosstool. Especially important, we can then flip it on for the standard library so that string_view becomes unsafe. That was a major oversight.

Interestingly (and I didn't expect this going in), because string_view is a template instantiation (and therefore pessimistically, but mistakenly, assumed to be owned by the current target), the unsafe_view attribute has to be added on the targets defining functions that accept a string_view parameter, not on cc_std itself. That's actually better for us, just wasn't what I expected at first when I was trying to test the CL.

Note that I tested it by only migrating one user of string_view to enable this -- it required unsafe, as expected (and I enforce this with a deny(unused_unsafe)). The other uses will, like the rest of google3, be migrated in a big ol' CL enabling the feature flag.

SLOPBOT WARNING: This change was pretty much entirely written by Gemini, with only minor cleanups by me. Quinn, you were wrong, sometimes it can handle big requests while I go afk! Ha ha ha ha productivity hack please don't take my job.

Prompt:

Add a new Crubit feature, as documented by third_party/crubit/g3doc/team/release.md, which is called unsafe_string_view. When this feature is enabled, classes with a gsl::Pointer attribute should be considered unsafe. This should break third_party/crubit/support/cc_std_impl/test/string_view/test.rs, and possibly other tests, which can be fixed by adding an unsafe block.

[...]

Instead of unsafe_string_view, can we rename this to unsafe_view? (And UnsafeView, and so on.) Please revise the implementation plan and let me look over it again.

[...]

Proceed.

PiperOrigin-RevId: 872978169
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant