Abseil Attributes

[Dana Jansens]

Hi,

third_party/abseil-cpp/absl/base/attributes.h has a bunch of macros that provide access to attributes in a cross-compiler-friendly manner.

I'd like to propose that we allow use of them all where they make sense in Chrome. I see we're already using a few: https://source.chromium.org/search?ss=chromium%2Fchromium%2Fsrc&q=ABSL_%20case:yes%20-f:third

If there is overlap with a base macro, the base one should be migrated to the abseil one, and this would be an agreement to support that.

I don't see the attributes helpers listed on https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++-features.md at all at the moment, but I can add something there.

Additional context in how this will help our nullability story: https://chromium-review.googlesource.com/c/chromium/src/+/3721480/comments/c7836aed_8d2e5fd0

Thanks,

Dana

[Avi Drissman]

The majority of the attributes in your codesearch link are ABSL_FALLTHROUGH_INTENDED and those should be migrated to [[fallthrough]].

The others I see are ABSL_ATTRIBUTE_RETURNS_NONNULL, ABSL_DEPRECATED, and ABSL_CONST_INIT.

ABSL_ATTRIBUTE_RETURNS_NONNULL is defined as __attribute__((returns_nonnull)). I could see using it.

ABSL_DEPRECATED is defined as __attribute__((deprecated(message))). IMO it would be better to directly use [[deprecated()]] which is C++14.

ABSL_CONST_INIT is defined as [[clang::require_constant_initialization]]. I could see using it.

In general, if something is offered by the standard, we should use the standard. If it is not, I could see us using the ABSL version. But I don't agree with a blanket allow of ABSL attributes.

Avi

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSvnXZ%3DnLZ4o3gnJp1jCgZSf%3D6QM7jUeDvbkwyMP0N%3D9w%40mail.gmail.com.

[Dana Jansens]

On Thu, Jul 7, 2022 at 10:01 AM Avi Drissman <a...@google.com> wrote:

The majority of the attributes in your codesearch link are ABSL_FALLTHROUGH_INTENDED and those should be migrated to [[fallthrough]].

The others I see are ABSL_ATTRIBUTE_RETURNS_NONNULL, ABSL_DEPRECATED, and ABSL_CONST_INIT.

ABSL_ATTRIBUTE_RETURNS_NONNULL is defined as __attribute__((returns_nonnull)). I could see using it.

ABSL_DEPRECATED is defined as __attribute__((deprecated(message))). IMO it would be better to directly use [[deprecated()]] which is C++14.

ABSL_CONST_INIT is defined as [[clang::require_constant_initialization]]. I could see using it.

In general, if something is offered by the standard, we should use the standard. If it is not, I could see us using the ABSL version. But I don't agree with a blanket allow of ABSL attributes.

Yes, I agree. I will rephrase. I would suggest that we allow the attributes but prefer C++ standard attributes over ABSL ones when they exist for backwards compat.

[Peter Kasting]

On Thu, Jul 7, 2022 at 7:10 AM 'Dana Jansens' via cxx <c...@chromium.org> wrote:

On Thu, Jul 7, 2022 at 10:01 AM Avi Drissman <a...@google.com> wrote:

The majority of the attributes in your codesearch link are ABSL_FALLTHROUGH_INTENDED and those should be migrated to [[fallthrough]].

The others I see are ABSL_ATTRIBUTE_RETURNS_NONNULL, ABSL_DEPRECATED, and ABSL_CONST_INIT.

ABSL_ATTRIBUTE_RETURNS_NONNULL is defined as __attribute__((returns_nonnull)). I could see using it.

ABSL_DEPRECATED is defined as __attribute__((deprecated(message))). IMO it would be better to directly use [[deprecated()]] which is C++14.

ABSL_CONST_INIT is defined as [[clang::require_constant_initialization]]. I could see using it.

In general, if something is offered by the standard, we should use the standard. If it is not, I could see us using the ABSL version. But I don't agree with a blanket allow of ABSL attributes.

Yes, I agree. I will rephrase. I would suggest that we allow the attributes but prefer C++ standard attributes over ABSL ones when they exist for backwards compat.

FWIW, I believe this is Abseil's position too -- ABSL_CONST_INIT is just "constinit" in C++20 and the documentation for it says if you know you're C++20+, just use that instead of the macro.

I'm OK with using these macros with the revised policy Dana suggests.

PK 

[Dana Jansens]

https://chromium-review.googlesource.com/c/chromium/src/+/3751442/

[Joe Mason]

Should we migrate any of the attributes in base/compiler_specific.h to the ABSL equivalents?

It looks like these have exact ABSL equivalents:

HAS_ATTRIBUTE

HAS_CPP_ATTRIBUTE

PRINTF_FORMAT (but not WPRINTF_FORMAT, which is a no-op anyway)

REINITIALIZES_AFTER_MOVE

ALIGNAS

These have ABSL equivalents that don't cover MSVC like the base ones do:

ALWAYS_INLINE

NOINLINE

And these have similarly-named ABSL macros with different implementations:

NOT_TAIL_CALLED vs ABSL_ATTRIBUTE_NO_TAIL_CALL

AnalyzerNoReturn() function vs ABSL_ATTRIBUTE_NORETURN

NO_SANITIZE(what) vs ABSL_ATTRIBUTE_NO_SANITIZE_ADDRESS, ABSL_ATTRIBUTE_NO_SANITIZE_MEMORY, etc

CONSTINIT vs ABSL_CONST_INIT (base is "__attribute__((require_constant_initialization))" and ABSL is "[[clang::require_constant_initialization]]", not sure the difference)

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQb0%3Dr4MXUqgBE2uRsrKCbPuboj-uQH6GfXAPw29Urgeg%40mail.gmail.com.