2
\$\begingroup\$

I'm trying to create a relatively simple class that stores global configuration in some serializable struct, and reads/writes from/to a file.

My main goal is to make this class easy to use correctly. While it's not a public library, there will be a sizable team using it in many parts of the codebase and I want to make it obvious to use in new code.

Settings.h

#pragma once

#include <exception>
#include <filesystem>
#include <fstream>
#include <iostream>
#include <shared_mutex>

/// Represents global configuration based on a struct.
/// Provides abilities to load/save to a file.
///
/// @param SettingsStruct The following global overloads must exist for
/// serialization to/from streams:
///
///   ostream& operator<<(ostream&, const SettingsStruct
///   istream& operator>>(istream&, SettingsStruct&);
///
template <typename SettingsStruct>
class Settings {
 private:
  SettingsStruct settings_;
  mutable std::shared_mutex mutex_;
  std::filesystem::path filename_;

 public:
  /// Attempts to open file and initialize in-memory settings.
  Settings(std::filesystem::path filename);

  /// @note Calls Commit() before destruction
  ~Settings() { Commit(); }

  Settings(const Settings &) = delete;
  Settings &operator=(const Settings &) = delete;
  Settings(Settings &&) = delete;
  Settings &operator=(Settings &&) = delete;

  /// Commits in-memory settings to a file
  void Commit() const;

  /// @return a copy of the current settings
  SettingsStruct Get() const {
    std::shared_lock lk(mutex_);
    return settings_;
  }

  /// Safely locks + mutates the current settings based on the given transform
  /// function.
  ///
  /// @return a copy of the mutated settings
  ///
  /// @example
  ///   settings.Mutate([](auto old){ old.SomeGroup.Nested = 4.0; return old; }
  template <typename Func>
  SettingsStruct Mutate(const Func &transformFunction) {
    std::scoped_lock lk(mutex_);
    settings_ = transformFunction(settings_);
    return settings_;
  }
};

template <typename SettingsStruct>
Settings<SettingsStruct>::Settings(std::filesystem::path filename)
    : filename_(filename) {
  std::ifstream ifs(filename);
  if (ifs.is_open()) {
    ifs >> settings_;
  } else {
    std::cerr << "warning could not open settings file "
              << std::filesystem::absolute(filename);
  }
}

template <typename SettingsStruct>
void Settings<SettingsStruct>::Commit() const {
  std::scoped_lock lk(mutex_);
  std::ofstream ofs(filename_);
  ofs << settings_;
}

main.cpp

#include <fstream>
#include <future>
#include <iostream>

#include "Settings.h"

using namespace std;

struct SomeGroupedSettings {
  float Nested = 0.1;  // default
  string Address = "asdf";
};

struct MySettings {
  int Version = 1;

  SomeGroupedSettings SomeGroup = {};
};

ostream &operator<<(ostream &out, const MySettings &s) {
  out << s.Version << '\n'
      << s.SomeGroup.Nested << '\n'
      << s.SomeGroup.Address << '\n';
  return out;
}

istream &operator>>(istream &in, MySettings &s) {
  in >> s.Version;
  in >> s.SomeGroup.Nested;
  in >> s.SomeGroup.Address;
  return in;
}

int main(int argc, char **argv) {
  const auto filename = argc > 1 ? argv[1] : "config.txt";

  // clean file for testing
  {
    ofstream ofs(filename);
    if (! ofs.is_open()) return 1;
    ofs << "1\n0.5\nold_address\n";
  }

  Settings<MySettings> settings(filename);

  cout << "Loaded settings:\n" << settings.Get() << endl;

  auto a = std::async(std::launch::async, ([&]() {
                        cout << "Changing address in other thread..." << endl;
                        settings.Mutate([](auto old) {
                          old.SomeGroup.Address = "new_address";
                          return old;
                        });
                        cout << "Changed address in other thread:\n"
                             << settings.Get() << endl;
                      }));

  cout << "Changing settings in other thread..." << endl;

  settings.Mutate([](auto old) {
    old.SomeGroup.Nested = 101.3;
    old.Version = 3;
    // leave SomeGroup.Address alone
    return old;
  });

  cout << "Changed settings in main thread:\n" << settings.Get() << endl;
}

Example output

Loaded settings:
1
0.5
old_address

Changing Nested in main thread...
Changing Address in other thread...
Changed Address in other thread:
3
101.3
new_address

Changed Nested in main thread:
3
101.3
new_address


With config.txt looking like:

3
101.3
new_address

Questions/not questions:

  • I will replace the <</>>-based serialization scheme with a parser based on Nlohmann's JSON Library in the future. I'm not looking for feedback on this.

  • Get() will be called often, while Mutate will be called rarely.

  • Performance is not a massive concern, but obvious improvements are obviously welcome.

  • The pattern for Mutate that I came up with (passing a mutator as an argument) doesn't seem widely used, or at least I'm not searching the right terms.

    • What are some better ways to handle mutation of shared classes?

    • Is the design of Mutate a terrible or particularly strange design? What are its pitfalls and why isn't it more common? I can't find much about it.

  • How could I add threadsafe move operations to this? It might be useful.

Any feedback, or examples of this sort of thing in the wild, is much appreciated.

\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

Design review

In summary, I don’t think this class is a good idea.

The first reason why is because it is not what it says it is. It calls itself a Settings class… but it has nothing at all to do with settings. It’s just a (putatively) thread-safe wrapper around some data that was read from a file. The only difference from any other thread-safe data wrapper is that it has a function to write the data back to the file. You could theoretically use it to read a plain string of text from a file, edit it, then write it back. (And yes, I get that operator>> will be replaced with a JSON parse, but that doesn’t really change the point. It would just mean that you could read any random JSON, edit it, then write it back… which is still not really a “settings” class.)

When the name of a class is wrong, it usually means that the concept is wrong, or at least badly muddled. That seems to be the case here. And when the concept is muddled, that usually means that the class is either difficult to use, or using it is inefficient. Let me illustrate.

Let’s say I want to read 3 related settings. So what is the obvious way to read 3 settings? Well, it’s something like this:

// assume:
// auto settings = Settings<YourActualSettingsMap>{filename};

auto setting_1 = settings.Get().setting_1;
auto setting_2 = settings.Get().setting_2;
auto setting_3 = settings.Get().setting_3;

Unfortunately, this very natural way of working with Settings is riddled with bugs and inefficiencies.

The first inefficiency is obvious: every time you call Get(), you copy the settings data.

Another inefficiency is much more insidious. Every time you call Get(), you have to acquire a lock on the internal mutex. That’s bad for efficiency, but what makes it dangerous is that it means that each of those three calls could be seeing entirely different views of the settings data.

Suppose it is very important that the 3 settings are in sync; they are all interrelated, so if one is changed, the other two usually need to be updated as well. So you read the first one… and then some other thread updates the settings data… and then you read the second one, and… well, now it is reading new data, so it might be out of sync with what you read for the first setting.

The problem exists on the other side, too. Once again, the natural way to update settings hides the trap:

settings.Mutate([](auto&& s) { s.setting_1 = "value 1"; return s; });
settings.Mutate([](auto&& s) { s.setting_2 = "value 2"; return s; });
settings.Mutate([](auto&& s) { s.setting_3 = "value 3"; return s; });

A naïve look at the code above would lead you to assume that you are changing the 3 settings together… but of course, since you release the lock between each call, anything could happen in between.

In both of the previous two code blocks, there is no indication at the call site that you are dealing with any locks at all. This is why it is a bad interface. You got the mantra half right: A good interface is easy to use correctly. But the important second half is: … and hard to use incorrectly. This interface is tricky use, and easy to use wrong.

Now, of course, you could rewrite both code blocks to be safe:

// when getting...
auto settings_view = settings.Get();
auto setting_1 = settings_view.setting_1;
auto setting_2 = settings_view.setting_2;
auto setting_3 = settings_view.setting_3;
// make sure users understand that if they make any changes to the settings
// view, they won't be saved!

// when setting...
settings.Mutate([](auto&& s) {
   s.setting_1 = "value_1";
   s.setting_2 = "value_2";
   s.setting_3 = "value_3";
   return s;
});

But neither of these patterns are obvious, or natural… especially the second one. You need to know the “tricks” to working with the class… you basically need to know the implementation.

There are other issues, but I’ll cover them later, throughout the review. For now, I’ll just reveal the punchline: Hiding locks (and other synchronization) from users is always a bad idea. At best it will be inefficient. At worst it will be dangerous.

I get it: non-concurrent code is simpler. It’s clearer, it’s easier to reason about, there are fewer traps and gotchas. I totally get the impulse to transform concurrent code into non-concurrent code. But you have to understand that when you disguise concurrent code as non-concurrent code, all of those traps and gotchas still exist. What’s changed is that the user is no longer aware of them. That’s not a good thing.

If some data needs to be locked, make sure the user is aware of it. Don’t hide the lock from them. You’re not helping them when you do that.

Here’s an alternative interface that doesn’t hide the lock:

// assume:
// auto settings = Settings{filename};

// ---------------------------------------------------------
// to read settings:

auto s = settings.lock();   // lock() returns a proxy object

auto setting_1 = s.get("setting-1");    // can only access settings via the
auto setting_2 = s.get("setting-2");    // locked proxy
auto setting_3 = s.get("setting-3");

// lock is released when proxy is destroyed

// won't work:
// settings.get("setting-1")    // ... need to lock to read anything
// s.set("setting-1", "value 1")    // ... lock() only gives you a read-only lock

// if you really only need a single setting:
// auto setting_1 = settings.lock().get("setting-1");

// ---------------------------------------------------------
// to set settings:

auto s = settings.lock_exclusive(); // or "lock_for_writing()" or whatever

s.set("setting-1", "value 1");
s.set("setting-2", "value 2");
s.set("setting-3", "value 3");

auto setting_1 = s.get("setting-1");    // can read from this kind of proxy

// again, lock released when proxy destroyed

This isn’t a perfect interface, because it is possible to create a deadlock by trying to acquire a lock while holding a lock. However, that’s just normal; everyone knows the danger of double-locking locks, and everyone knows that you should never hold a lock any longer than you need to, and so on. When the lock is blatant and clear in the code, normal programmer discipline can come into play. When the lock is hidden… not so much.

Incidentally, the proxy pattern means you can’t use bare structs as your settings structures. That’s why I switched to function calls rather than data member accesses in the code block above. It sucks, but there’s just no practical way to do it without the proposed operator. or something like it. If you want to use a bare struct, then the lock will have to be separate from the data (unlike in the code above, where the lock and the data proxy are one and the same); that just makes things much more dangerous.

Another option is to take a cue from the “RCU”, or “read-copy-update”, pattern. Basically, you use shared pointers, and whenever you “get” the settings object to work with, you actually get a shared pointer to it. When you mutate the settings, you just mutate your own private copy of the data, and then replace the pointer in the main object:

// assume:
// auto settings = Settings<MySettings>{filename};
// internally, settings allocates a shared_ptr<MySettings>

// ---------------------------------------------------------
// to read settings:

auto s = settings.Get();    // returns a shared_ptr<const MySettings>
auto setting_1 = s->setting_1;
auto setting_2 = s->setting_2;
auto setting_3 = s->setting_3;

// or:
auto setting_1 = settings.Get()->setting_1;
auto setting_2 = settings.Get()->setting_2;
auto setting_3 = settings.Get()->setting_3;

// doesn't matter how many threads are reading settings; each just gets a
// pointer to the data.
//
// doesn't matter another thread *changes* the settings while you're reading,
// because you have your own private view (until the shared pointer you have
// is destructed).

// can't change the settings, because it is a pointer to const:
// settings.Get()->setting_1 = 0;   // won't compile

// ---------------------------------------------------------
// to set settings:
auto s = settings.Copy();   // allocates a NEW shared_ptr<MySettings> by
                            // copying the internal settings data

// do whatever you want safely, because you have your own copy
s->setting_1 = 0;
s->setting_2 = 0;
s->setting_3 = s->setting_1;    // can also read settings

// in order to make your changes visible to other threads, and when saving to
// file, you need to apply them:
settings.Reset(*s); // or Update() or whatever you want to call the function

You still need to synchronize all those shared pointer copies and resets, unless you’re using C++20 and can use std::atomic<std::shared_ptr<MySettings>>. Pre-C++20:

template <typename SettingsStruct>
class Settings
{
    std::shared_ptr<SettingsStruct> p_settings_;
    mutable std::shared_mutex mutex_;
    std::filesystem::path filename_;

public:
    // ... [snip] ...

    auto Get() const
    {
        auto _ = std::shared_lock{mutex_};
        return p_settings_;
    }

    auto Copy()
    {
        auto _ = std::shared_lock{mutex_};
        return std::shared_ptr<SettingsStruct const>{p_settings_};
    }

    auto Reset(SettingsStruct const& s)
    {
        auto p = std::make_shared(s);

        auto _ = std::scoped_lock{mutex_};
        p_settings_ = p;
    }

    auto Reset(SettingsStruct&& s)
    {
        auto p = std::make_shared(std::move(s));

        auto _ = std::scoped_lock{mutex_};
        p_settings_ = p;
    }
};

With C++20:

template <typename SettingsStruct>
class Settings
{
    std::atomic<std::shared_ptr<SettingsStruct>> p_settings_;
    std::filesystem::path filename_;

public:
    // ... [snip] ...

    auto Get() const
    {
        return p_settings_.load(); // possibly with memory order acquire
    }

    auto Copy() const
    {
        // this is safe, because the owned, pointed-to object is never
        // mutated (the *pointer* is sometimes mutated (atomically, so
        // safely), but the *pointed-to data* is never mutated), so we can
        // copy it without needing to lock
        return std::make_shared<SettingsStruct>(*Get()));
    }

    auto Reset(SettingsStruct const& s)
    {
        auto p = std::make_shared<SettingsStruct>(s);

        // no problem, atomic
        p_settings_ = p;
        // or: p_settings_.store(p); // possibly with memory order release
    }

    auto Reset(SettingsStruct&& s)
    {
        auto p = std::make_shared<SettingsStruct>(std::move(s));

        // no problem, atomic
        p_settings_ = p;
        // or: p_settings_.store(p); // possibly with memory order release
    }
};

This pattern is actually easy to make movable, too, if you really want to. Because everyone is working with their own private view, it doesn’t really matter what you do to the main object. So long as you don’t mutate the pointed-to value (though you can replace the pointer, as shown), everything is safe.

The Mutate() problem

The reason you probably haven’t seen the pattern you’re using for Mutate() very often is because it is an anti-pattern.

It is always a terrible idea to let random code execute while holding a lock. That’s what you’re doing when you allow transformFunction to run while mutex_ is locked. You assume the user isn’t going to do anything stupid or reckless; you assume all they’re going to do is set some fields in the struct then return. But it is very trivial to violate your assumptions, and, worse… it’s not even that wild or silly.

For example, let’s say I want to set one setting based on the value of another:

settings.Mutate([](auto&& s) {
   s.value = settings.Get().other * 2; // boom
   return s;
});

I know you intend for the user to do s.value = s.other * 2;… but there is no way to prevent misuse, and it’s such an easy mistake to make. It could be even trickier if the code is s.value() = calculate_value();, and somewhere deep in the call to calculate_value() is a call to Get().

Just don’t do it: don’t let random user code run while holding a lock.

Note that you could fix Mutate() by doing something like this:

template <typename Func>
SettingsStruct Mutate(const Func &transformFunction)
{
    // don't really need optional, just using it to keep the code simple
    auto s = std::optional<SettingsStruct>{};

    // copy settings, then release the lock
    {
        auto _ = std::shared_lock{mutex_};
        s = settings_;
    }

    // do the transform on the copy
    *s = transformFunction(*s);

    // now change the actual settings
    auto _ = std::scoped_lock{mutex_};
    settings_ = *s;
    return settings_;
}

But holy wow is that wildly inefficient for just changing a setting or two. This is a sign that your abstraction is wrong.

Code review

#pragma once

Don’t use #pragma once. It’s ‘okay-ish’ (widely supported though non-standard) when it works. When it fails… it is absolute nightmarish catastrophe. Pray you never experience a failure of #pragma once.

  Settings(const Settings &) = delete;
  Settings &operator=(const Settings &) = delete;
  Settings(Settings &&) = delete;
  Settings &operator=(Settings &&) = delete;

The convention in C++ is to put the & (or &&) with the type.

In other words:

  • Settings &operator=(const Settings &) = delete;
  • Settings& operator=(const Settings&) = delete;
  • Settings& operator=(Settings const&) = delete; (east const style)
  • auto operator=(Settings const&) -> Settings& = delete; (east const plus trailing return)
  SettingsStruct Get() const {
    std::shared_lock lk(mutex_);
    return settings_;
  }

There is no conceivable situation where a level-headed programmer would want to call Get(), and not want to use the returned settings struct. (As opposed to, say, Mutate(), which also returns a copy of the settings, but a user might not care… as in your example code.) This is a good place for [[nodiscard]].

  template <typename Func>
  SettingsStruct Mutate(const Func &transformFunction) {
    std::scoped_lock lk(mutex_);
    settings_ = transformFunction(settings_);
    return settings_;
  }

I mentioned above the problems that come with allowing user code to execute while holding the lock, and the hoops you have to jump through to avoid that. If you think about it logically, the only thing that actually needs to be protected by the lock is the assignment to settings_ (except, of course, you want to pass settings_ or a copy of it to the function first, too).

Taking the function by const reference is unnecessarily constraining. It “works” with simple lambdas because operator() is const-qualified by default for lambdas. But if I use a function object, or a mutable lambda, boom.

The best solution here in any case is really to just use a forwarding reference.

template <typename SettingsStruct>
Settings<SettingsStruct>::Settings(std::filesystem::path filename)
    : filename_(filename) {
  std::ifstream ifs(filename);
  if (ifs.is_open()) {
    ifs >> settings_;
  } else {
    std::cerr << "warning could not open settings file "
              << std::filesystem::absolute(filename);
  }
}

Taking the path by value is fine, since you’re storing it anyway, but once you’ve got it, you should move it rather than copying it again:

Settings<SettingsStruct>::Settings(std::filesystem::path filename)
    : filename_(std::move(filename)) {

Now, inside the constructor, you check that the file is open, which is fine… but you never check anything else. What if the settings file data is corrupt and fails to read? What if the file is fine, but there’s a disk error while reading? And even if the file doesn’t open, you just chug along merrily anyway. What if it really matters to me that I’ve properly read the user’s desired settings?

This is a job for exceptions. Unfortunately, you need to actually turn them on in iostreams (because they’re ancient, from pre-exception days):

template <typename SettingsStruct>
Settings<SettingsStruct>::Settings(std::filesystem::path filename)
    : filename_{std::move(filename)}
{
  std::ifstream ifs{filename_};
  ifs.exceptions(std::ifstream::badbit | std::ifstream::failbit);
  ifs >> settings_; // will throw if the file is not open or unreadable, or
                    // if the file's contents are corrupt
}

I know your ultimate goal is to use a JSON load function, so the actual details of making iostreams throw exceptions isn’t the point here. Rather, the point is about checking for and reporting errors generally. Exceptions are your only option for reporting failure from a constructor. The alternative is to have the constructor only default construct the settings, and have a separate load function which could throw or return bool or whatever you like. It’s probably better to have a separate load function in any case, because the user might want to refresh the settings if some external force has changed them (or they might just want to try loading a settings file, to see if it’s legit).

And finally…

template <typename SettingsStruct>
void Settings<SettingsStruct>::Commit() const {
  std::scoped_lock lk(mutex_);
  std::ofstream ofs(filename_);
  ofs << settings_;
}

There is no need to hold the lock while opening the file. Again, I know your ultimate goal is to ditch the iostreams stuff, but the points I’m making are not iostreams-specific. You should always do the minimal amount of work necessary while holding a lock. If your library has separate functions to open files, format data as JSON, and write files, then only the formatting part needs to be protected by the lock. Indeed, you could abandon the lock entirely at the cost of an extra copy of the data if you did:

template <typename SettingsStruct>
void Settings<SettingsStruct>::Commit() const {
  your_write_function(Get());

  // for example:
  //    auto ofs = std::ofstream{filename_};    // no problem if filename is never mutated
  //    ofs << Get();
}

The other problem is that there is no error reporting at all. How would the user know the file was opened and successfully written to? That’s kinda important to know, if only to report to the user that their settings weren’t saved.

I’m not going to review the test/example code (even though there are some dire problems with it… using namespace std;, and endls all over the place…), because I doubt you’re interested in it.

Summary

Hiding locks from users is evil. Don’t be evil.

Allowing arbitrary code to execute while holding a lock is extremely risky. Avoid it unless ABSOLUTELY necessary (which it absolutely isn’t in this case).

A class whose only purpose is to provide locked access to another class seems specious. I suppose it would be possible if one could write proper general proxy objects in C++… but you can’t. (A proper general proxy object might have an overloaded operator. that just calls the wrapped class’s operator. while locked… but we don’t have overloadable operator., so… 🤷🏼.)

So you have to give up the dream of Settings<SettingsStruct> working with any random SettingsStruct type. Either make Settings specific to one specific SettingsStruct type, or use an interface… which will limit which types you can use as SettingsStruct, but still allow some options, although you won’t be able to support bare structs that only have data members.

\$\endgroup\$
7
  • \$\begingroup\$ I agree to most of it (I think I was trying to kinder in my review but perhaps this is better). I do not agree with the overall assessment though. I know it is very difficult to come up with very secure design even with best intentions (auto_ptr) mainly because the C++ deals with new language features. They seem to go on adding new things which ends up being more difficult to have perfect design. I am not fan of documentation because it doesn't keep up with code but perhaps giving very limited API with strong warning in doc might help. Just a thought \$\endgroup\$
    – user244660
    Jul 8, 2021 at 23:28
  • \$\begingroup\$ Fantastic info about API design especially, thank you. I am slowly abandoning my dream of allowing arbitrary structs as configs. Maybe string keys is the simpler way. What are the pitfalls of using the proxy-object method, but overloading operator-> or just a Get() method that returns a reference to the underlying struct from the proxy? Would it be unsafe because you can copy the reference outside of the proxy's lifetime? \$\endgroup\$
    – MHebes
    Jul 9, 2021 at 3:18
  • \$\begingroup\$ Tried it out. The temporary lifetime of the proxy object in the method call makes things like settings.Lock()->Version++ unsafe. \$\endgroup\$
    – MHebes
    Jul 9, 2021 at 4:39
  • \$\begingroup\$ Great review, as usual. One thing you missed mentioning from Commit() is that it truncates the file before starting to write. So if anything goes wrong during the write, we no longer even have the last good version. I hate it when programs lose their settings for no apparent reason (Qt Assistant, I'm looking at you!). \$\endgroup\$ Jul 9, 2021 at 6:33
  • \$\begingroup\$ The RCU pattern seems great, but it does suffer from two threads erasing each others changes if they both try the Copy/Reset at the same time. I guess it would be unlikely, though. \$\endgroup\$
    – MHebes
    Jul 9, 2021 at 21:26
2
\$\begingroup\$

SettingsStruct Get() const {

You do not need to return copy. If someone want to make copy, they can still do it even if you return const SettingsStruct&. Returning copy would affect performance (a lot if Get is called way too many times)

The pattern for Mutate that I came up with (passing a mutator as an argument) doesn't seem widely used, or at least I'm not searching the right terms. I

The reason probably you did not come across many because eventually they would become difficult to maintain. You are giving access to a some function which may be changing state that would affect Settings object. If you are going to provide this to limited number of users, it would be fine I am sure.

What are some better ways to handle mutation of shared classes?

I am sure you are realize that in a way you are giving templated serialization version for a file, any file. Nothing wrong with that; just that if you wanted you could look at some serialization frameworks for inspiration. I wouldn't call them better just different.

Is the design of Mutate a terrible or particularly strange design? What are its pitfalls and why isn't it more common? I can't find much about it.

No, it not terrible design at all. It is useful. All you need to remember is, there is very little you can do to have specific restrictions on what it may or may not do. For example, what happens if it throws? Do you want to change the accepted signature to noexcept? Is Mutate reentrant?

How could I add threadsafe move operations to this? It might be useful.

I would suggest you get this one out first, let some people use it before adding more capabilities. You would get better feedback (from yourself if you not from others).

One additional feedback:

void Settings<SettingsStruct>::Commit() const {

This should not be constant. The reason why it can be constant is because you are not changing data that is owned by the class but you are doing much more. You are changing underlying storage. User of the class should have very clear boundary of what he can or cannot do.

\$\endgroup\$
7
  • \$\begingroup\$ Get has to return a copy. If you return a reference, you expose the internal data without a lock. That kinda defeats the whole point of the class. \$\endgroup\$
    – indi
    Jul 8, 2021 at 9:47
  • \$\begingroup\$ @indi You are taking responsibility of something that you should not take. It's job of the developer of SettingStruct to allow or disallow access. Yours is just to make sure that when it is accessed in the way you intended you don't give incorrect write access. Or this class becomes too tightly coupled with SettingStructs. Imagine having huge amount of data in that struct. Every time you copy, you are going to increase overhead. \$\endgroup\$
    – user244660
    Jul 8, 2021 at 9:57
  • \$\begingroup\$ That makes no sense. If SettingStruct is already thread-safe, then this entire class has no point. If it is not thread-safe, then you can’t return a reference. So Get() either has to return a copy, or the whole Settings class should be thrown out. This is not a question of efficiency or overhead, it is a question of correctness. \$\endgroup\$
    – indi
    Jul 8, 2021 at 19:33
  • \$\begingroup\$ @vish I believe what indi is trying to say is that the copying from reference will not be under mutex lock, so not atomic. Other thread may modify the data before the copy constructor finishes execution. \$\endgroup\$ Jul 8, 2021 at 20:01
  • \$\begingroup\$ @indi I am not asking to return by reference I am asking to return by const reference. You can still make copy if you want to, is it not? Incomputable, does that make sense? \$\endgroup\$
    – user244660
    Jul 8, 2021 at 22:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.