isocpp / CppCoreGuidelines

The C++ Core Guidelines are a set of tried-and-true guidelines, rules, and best practices about coding in C++
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
Other
42.73k stars 5.44k forks source link

Q: How to express that something is accessed for its side effect in C++? #1048

Closed franzhollerer closed 7 years ago

franzhollerer commented 7 years ago

This question is derived from the exception in MISRA C 2017, Rule 2.2.

A cast to void is assumed to indicate a value that is intentionally not being used. The cast is therefore not dead code itself. It is treated as using its operand which is therefore also not dead code.

I would like to know how to do this in C++, as C-style cast are discouraged.

Example:

extern volatile uint32_t* uart_rx_reg;

void foo()
{
    // do something here
    :
    // uart_rx_reg is accessed for its side effect to empty the receive register
    (void) *uart_rx_reg;    // read and discard received character
}

Is static_cast the right choice, or are there better ways to express this?

MikeGitb commented 7 years ago

There should be an attribute for this.

franzhollerer commented 7 years ago

There should be an attribute for this. Many thanks @MikeGitb for the quick response. Unfortunately, I did not get the point. Could you please provide an example.

JonasToth commented 7 years ago

I am not 100% sure, but maybe he means this: http://en.cppreference.com/w/cpp/language/expressions#Discarded-value_expressions http://en.cppreference.com/w/cpp/language/attributes see [[nodiscard]]

arvidn commented 7 years ago

@franzhollerer I take it the question really is about how to suppress warnings about unused variables. The fact that uart_rx_reg is volatile indicates that the read is an observable side effect.

If the remaining issue is just to suppress the warning, you may change the cast to a c++-style cast, like this:

static_cast<void>(*uart_rx_reg);

EDIT:

actually, I wouldn't expect that statement to issue a warning in the first place. Does it?

jwakely commented 7 years ago

I would like to know how to do this in C++, as C-style cast are discouraged.

(void)expr; is not really a cast, since it's not converting anything.

Using (void)e exactly the same way is fine IMHO, or static_cast<void>(e) if you prefer. I'm not sure this deserves a guideline.

MikeGitb commented 7 years ago

(void) *uart_rx_reg;

As it turns out, that line does NOT force clang to perform a read at the specified address (just look at the assembly). You have to assign the result to a variable.

arvidn commented 7 years ago

interesting. at least it produces a warning. https://godbolt.org/g/Ts3L3P

franzhollerer commented 7 years ago

That's very intersting, even slightly off-topic for the CPP Core Guidelines. Thank you for pointing that out.

Unfortunately, I am not that familiar with the C++ standard, but the ISO/IEC 9899:1999 C standard states:

6.3.2.2 void
The (nonexistent) value of a void expression (an expression that has type void) shall not
be used in any way, and implicit or explicit conversions (except to void) shall not be
applied to such an expression. If an expression of any other type is evaluated as a void
expression, its value or designator is discarded. (A void expression is evaluated for its
side effects.)

Is my code wrong, or is it an abnormality of clang? I used the (void) cast sometimes on embedded system for forcing a read form a peripheral register. I thought it is a nice way to state that the expression is evaluated for its side effects only. And I felt comfortable to be in line with MISRA. Probably I failed to catch and understand the details :-(

MikeGitb commented 7 years ago

The void cast is perfectly fine. Clang seems to disagree with the other compilers if just dereferencing a volatile pointer (as opposed to copy the value stored at that address) requires the program to actually read from that memory location. And yes, it is OT, but I wanted to point out that no obvious pitfall, before it ends up in an example on the guidelines.

AndrewPardoe commented 7 years ago

Per our editor's discussion, our advice in ES.48 should allow for the usage of a C-style cast to (void) in this situation. See 10.6.7 in the standard.

By the way, volatile is I/O. If your compiler is warning about that it seems to be a bug in your compiler.

hsutter commented 7 years ago

There are three cases covered by this issue:

  1. volatile (as in the top example): We are taking no action. In the example at top, you do not need any cast at all. As noted above, C++ (and C) compilers should never warn about "apparently unused" reads from volatile variables. If yours does, submit it as a bug to your compiler vendor. (I do not have a copy of MISRA C:2012, the latest, to see if their examples include volatile, but I hope they don't because volatile accesses are never dead code.)

  2. [[nodiscard]]: More specifically, our decision was to update ES.48 to allow (void) to turn off [[nodiscard]] warnings, and those only. (This is the way the C++ standard itself says to turn off [[nodiscard]] warnings.)

  3. Other expressions that look like dead code: We are taking no action. At this point we don't want to change to the Guidelines to bless using a cast to shut up other compiler warnings. If after addressing volatile and [[nodiscard]] as above, you feel we nevertheless should reconsider doing more for the general case, please reopen a new issue.

Thanks!

MikeGitb commented 7 years ago

@hsutter: IIRC, the warning by clang is justified, because it does NOT generate a read instruction unless you assign the result of *uart_rx_reg to some variable. I don't know if this is a known bug in clang or due to insufficient specification in the standard.

franzhollerer commented 7 years ago

If yours does, submit it as a bug to your compiler vendor. (I do not have a copy of MISRA C:2012, the latest, to see if their examples include volatile, but I hope they don't because volatile accesses are never dead code.)

@hsutter: Below screenshots from MISRA C 2012, Rule 2.2 on page 41 and 42

misra_c_2012_rule2 2_page_41

misra_c_2012_rule2 2_page_42

hsutter commented 7 years ago

(Updated with information from Clang guru @zygoloid, thanks Richard)

@MikeGitb: See the repro here: https://godbolt.org/g/GhtMYg . Note that GCC, CL (MSVC), and ICC all perform the read even at the highest optimization levels, but Clang omits the read even at default optimization levels... but only in C++98 mode (still the default I guess). So this appears to be a problem only in Clang in C++98 mode.

The best fix for Clang is to use the flag -std=c++11 (or later).

Alternatively, if you must use Clang in C++98 mode, you can change this line

v;

to

int work_around_clang_cpp98_mode = v;

or to even just

v+0;

Personally I think it would be good for Clang to just make this work even in C++98 mode, as it's still conforming C++98, is exactly zero cost in the vast majority of programs (which don't do this), and improves compatibility with moving code to Clang from other compilers (especially GCC, with which Clang usually goes to great lengths to be compatible).