Modern C++ Example on a class

[meteor1203]

Hello,

I'm trying to figure out how code is recommended to write with the C++17 standard.
I am reviewing code of a little library I wrote for personal use and because I want to use it as a c++ reference in the future, I want to make sure it's 100% up to todays' c++ standard.
I have been doing a lot of research, but at the end I have been left confused about a few things. Let me demonstrate this on this example class with a private field and a public set and get function:

  class Foo
{
    int a = 4;

public:
   [[nondiscard]] int GetA() const noexcept;
   void SetA(int a) noexcept;
 
};

My question is: What keywords are actually recommended to use? In theory they all match. Const because no field is manipulated. Noexception because a simple return of a private variable can not cause an exception. [[NonDiscard]] because getting the variable but not using it makes no sense; the call would be useless.

While that backs the theory of these keywords I have seen other posts where they recommend to not use [[nondiscard]] because it makes the header file so loaded, which is also one of my concerns. The get function here already has 3 keywords and who knows maybe you could possibly find a 4th in the crazy world of c++.

Looking for Get And Set functions online the const keyword seems to be enough though. At the same time I think you can't argue the discard and noexcept keywords are wrong? I noticed that most of the built in math functions like sin also use these.

Another question I have is: Do Set and Get functions still need to get defined in the cpp file? I have seen other (production) code where they are directly defined in the header and these functions are just the obvious one liners. Wouldn't defining it in the header encourage bloating?

I might be massively overthinking here but I just want to get it right.

[TheIdeasMan]

@meteor1203

It is
[[nodiscard]] not [[NonDiscard]] or [[nondiscard]]

https://en.cppreference.com/w/cpp/language/attributes

cppreference puts the attributes on the line before the function declaration - I like this as a personal preference.

One could also use [[noreturn]] on a void function.

One of the reasons for attributes is to help the compiler produce a message when the coder makes a mistake. The more one can get the compiler to help oneself, the better off one will be.

A set function generally cannot be const unless you are going to use the mutable
https://en.cppreference.com/w/cpp/language/cv

With noexcept, make sure that the function does not have anything that will throw, so that you don't get an unexpected std::terminate call.
https://en.cppreference.com/w/cpp/language/noexcept_spec

[TheIdeasMan]

With defining functions inline in the header file, that is fine as long as the code is less than what the overhead of the function call is. So functions that literally return a value or assign one value would be fine.

https://en.cppreference.com/w/cpp/language/inline

[meteor1203]

Thank you for your help! So it's perfectly fine to use all of these and it's not "too much"?
Regarding the function definitions in the header file I found out today that this simply makes them implicitly inlined, which is possible for such simple set and get functions.

[TheIdeasMan]

meteor1203 wrote:

So it's perfectly fine to use all of these and it's not "too much"?

Yes, in the cppreference for attributes one can see some of the attributes available for the gcc compiler, shown as [[gnu::hot]] for example. At the bottom of that page there is a link to attributes supported by the clang compiler which includes all the gcc ones as well.

Note that if an attribute is not recognised by the compiler it is simply ignored with no diagnostic, so the [[nondiscard]] you had earlier would not have done as you intended.

Another reason for attributes is they may help the compiler do optimisations.

Good Luck with your coding :+)

It may be worthwhile posting some more of it here, you may surprised what you can learn from others on this forum.

[jonnin]

So it's perfectly fine to use all of these and it's not "too much"?

I see these as visual clutter, and would not over-do it. Its not too much for the compiler, but I wouldn't want to read code with too much of it.

[dhayden]

[[NonDiscard]] because getting the variable but not using it makes no sense; the call would be useless.

By that logic, all discarded values should generate a warning. After all, what makes yours special?

The thing here is that you, the class author, don't know all the ways and reasons that someone will use your class. There are probably reasons someone would want to discard the value of the call. You just haven't thought of them.

Noexception because a simple return of a private variable can not cause an exception

I assume you're thinking the same for setA() too. But in that case, why have the accessor methods at all? Why not just make a public?

The usual reason to keep the accessors in this case is because you might decide to change the implementation in the future. For example, you might decide that a should be computed from other values instead of stored. A classic example of this is the left side, right side, and width of a window. You only need to store 2 of those and you can compute the third.

So if the reason for the accessor is because you might change the implementation then who's to say that a new implementation won't throw an exception? If it does then you'll need to change the attributes of the method, which somewhat defeats the purpose of having the method in the first place.

[seeplus]

If a method is marked as noexcept, then the compiler can perform additional performance optimisation as the stack doesn't need unwinding when an exception occurs - as none will. Since C++17 noexcept is part of the function type.

Also containers such as std::vector will move their elements if the elements' move constructor is noexcept, and copy otherwise.

So IMO use noexcept when an exception cannot be thrown.

[mbozzi]

While that backs the theory of these keywords I have seen other posts where they recommend to not use [[nondiscard]] because it makes the header file so loaded

You might want your code to be nice, but you're not paid to produce pretty code. Instead, you're paid to produce correct software, and do so on time and on budget. This is the measure by which you need to evaluate decisions. None of your users know or care what your source code looks like: it's immaterial to your bottom line.

Broad user experience shows that nodiscard locates software defects. This crucial and direct benefit comes at the trivial cost of... typing a little more?

Rejecting nodiscard is bad engineering.

There are probably reasons someone would want to discard the value of the call. You just haven't thought of them.

In that unlikely case, the caller may silence the warning by converting the result to void.
static_cast<void>(get_x())
or in this special case, even a C-style conversion is harmless
(void)get_x()

Noexception because a simple return of a private variable can not cause an exception

The standard library's usage rationale regarding noexcept is a good case study.
http://www.cplusplus.com/forum/general/256009/#msg1121235

However, your code is not the standard library. Take this into account.

Get and Set functions [...]

Don't write these trivial getters and setters. The usual excuse for writing do-nothing functions is to "increase flexibility". However, there are very few changes you can make to Foo (other than additions) that don't require you to review every location the interface is used.

You gain nothing from trivial getters and setters until you make such a change. In exchange for a potential future benefit that rarely manifests, engineers must look up GetA in order to ensure its preconditions (if any exist) are met at the call-site, to verify properties like thread-safety, to check its exception-safety guarantees, and to make sure they respect any special caveats involving its use.

Just say no. Quit wasting time and use the plain struct
struct point { int x_, y_; };

Exceptions to this guideline can involve interfaces with representations that might change very frequently, or interfaces with a stronger requirement for stability. Most classes, most code, only sees local use within a unit or subsystem of a larger program. Such code needs minimal encapsulation, because it will be easier to adjust the uses of that interface than trying to design the interface to allow the change without breaking existing code.

Another question I have is: Do Set and Get functions still need to get defined in the cpp file? I have seen other (production) code where they are directly defined in the header and these functions are just the obvious one liners. Wouldn't defining it in the header encourage bloating?

Functions inside a class definition are implicitly inline. The contents of inline functions are part of the ABI. noexcept is also part of the ABI.

I might be massively overthinking here but I just want to get it right.

Nah, this is a good question.