[PATCH] D151187: [doc] Add casting style preference to coding standards

[James Henderson via Phabricator via llvm-commits]

jhenderson marked an inline comment as done.
jhenderson added a comment.

In D151187#4364070 <https://reviews.llvm.org/D151187#4364070>, @barannikov88 wrote:

> In D151187#4363988 <https://reviews.llvm.org/D151187#4363988>, @jhenderson wrote:
>
>> In D151187#4363915 <https://reviews.llvm.org/D151187#4363915>, @barannikov88 wrote:
>>
>>> What about function style casts? I personally find them acceptable in limited cases, such as "constructing" int64_t from unsigned.
>>
>> It's a fair question, and I don't have a good answer for this. Strictly, we should //probably// forbid them in new code on the basis that they are just as bad as C-style casts (they are explicitly defined to be identical), with regards to safety etc, but I also have used them in the past for the same "constructing" style like you find. Any suggestion on how we might codify the latter.
>
> Maybe adopt this clang-tidy check? https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-cstyle-cast.html
> That is, allow C/functional-style cast when it is known to be safe (like unsigned -> int64_t or enum -> int), and prefer named casts otherwise.
> The check also provides fix-its, which is nice.

The issue here is that if a C-style/functional-style cast is added here, and then one of the types changes, the cast will prevent a potential build breakage. C++-style casts don't completely fix that, but should prevent some mistakes at least.



================
Comment at: llvm/docs/CodingStandards.rst:1269
+
+When casting, use ``static_cast`` and ``reinterpret_cast`` rather than C-style
+casts. The sole exception to this is when casting to void to suppress warnings
----------------
nhaehnle wrote:
> const_cast for completeness? And perhaps a good place to reiterate that dynamic_cast is explicitly *not* accepted, in favor of LLVM's own casting infrastructure.
Makes sense.


================
Comment at: llvm/docs/CodingStandards.rst:1270
+When casting, use ``static_cast`` and ``reinterpret_cast`` rather than C-style
+casts. The sole exception to this is when casting to void to suppress warnings
+about unused variables, when C-style casts should be preferred instead:
----------------
aaron.ballman wrote:
> FWIW, I think it would be reasonable to also mention use of `[[maybe_unused]]` as a potential alternative to casting to `void`, but without prejudice as to which approach to use.
I feel like any reference to `[[maybe_unused]]` belongs either in its own section (which I don't propose to add at this time) or in the section above where asserts are discussed, as that talks about casting to void to suppress the warnings. What I want to achieve here is to ensure the rule for preferring C++-style doesn't apply to that specific case. `[[maybe_unused]]` is an alternative solution, but isn't relevant to casting.


================
Comment at: llvm/docs/CodingStandards.rst:1274-1276
+  int x = 42;
+  long *y = reinterpret_cast<long *>(&x);
+  (void)y;
----------------
aaron.ballman wrote:
> How about a more realistic example along the lines of:
> ```
> int check_value = doSomeWorkWithSideEffects();
> assert(check_value == 42 && "the assert is removed in NDEBUG builds");
> (void)check_value;
> ```
I want to avoid essentially repeating the example or explanation from the previous section about asserts. I'm tempted to drop the void cast example entirely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151187