felipecrv commented on code in PR #41021: URL: https://github.com/apache/arrow/pull/41021#discussion_r1556368340
########## cpp/src/arrow/util/macros.h: ########## @@ -36,40 +36,63 @@ TypeName& operator=(TypeName&&) = default #endif -#define ARROW_UNUSED(x) (void)(x) -#define ARROW_ARG_UNUSED(x) +// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is +// not likely to be taken (for instance, a CHECK failure), and use that information in +// static analysis. Giving the compiler this information can affect the generated code +// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how +// this feature can be used to improve code generation. It was written as a positive +// comment to a negative article about the use of these annotations. // -// GCC can be told that a certain branch is not likely to be taken (for -// instance, a CHECK failure), and use that information in static analysis. -// Giving it this information can help it optimize for the common case in -// the absence of better information (ie. -fprofile-arcs). +// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression +// is true, without evaluating it, and to optimise based on this assumption [2]. +// If this condition is violated at runtime, the behavior is undefined. This can +// be useful to generate both faster and smaller code in compute kernels. Different +// optimisers are likely to react differently to this annotation. It should be used +// with care [3] when we can prove by some means that the assumption is guaranteed +// to always hold. // +// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc +// [2] "Portable assumptions" +// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf +// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic" +// https://blog.regehr.org/archives/1096 +#define ARROW_UNUSED(x) (void)(x) +#define ARROW_ARG_UNUSED(x) #if defined(__GNUC__) +// Both GCC and clang define __GNUC__ Review Comment: I changed the comment :) ########## cpp/src/arrow/util/macros.h: ########## @@ -36,40 +36,63 @@ TypeName& operator=(TypeName&&) = default #endif -#define ARROW_UNUSED(x) (void)(x) -#define ARROW_ARG_UNUSED(x) +// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is +// not likely to be taken (for instance, a CHECK failure), and use that information in +// static analysis. Giving the compiler this information can affect the generated code +// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how +// this feature can be used to improve code generation. It was written as a positive +// comment to a negative article about the use of these annotations. // -// GCC can be told that a certain branch is not likely to be taken (for -// instance, a CHECK failure), and use that information in static analysis. -// Giving it this information can help it optimize for the common case in -// the absence of better information (ie. -fprofile-arcs). +// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression +// is true, without evaluating it, and to optimise based on this assumption [2]. +// If this condition is violated at runtime, the behavior is undefined. This can +// be useful to generate both faster and smaller code in compute kernels. Different +// optimisers are likely to react differently to this annotation. It should be used +// with care [3] when we can prove by some means that the assumption is guaranteed +// to always hold. Review Comment: I greatly expanded the comments here. Pushing now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
