Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Const-correctness #14

Open
hwiedPro opened this issue Mar 23, 2020 · 5 comments
Open

Const-correctness #14

hwiedPro opened this issue Mar 23, 2020 · 5 comments
Assignees

Comments

@hwiedPro
Copy link

Hi please check const-correctness. It is not given e.g. for getString

@malter
Copy link
Member

malter commented Mar 26, 2020

That is not that easy because the first get call on a ConfigItem defines its type if not already defined. Thus it isn't a const call. What is your use case where you have trouble?
An ofter valid workaround (not the most performant way) is to create a local copy and perform the get call on the copy.

@hwiedPro
Copy link
Author

I see, if the type is stored in a class variable, can't that variable be defined as mutable?

I wanted to declare a ConfigMap as const as i wanted to provide a possibility to read a classes config, without allowing to change it. Therefore I wanted to return a const& of the private ConfigMap.

@malter
Copy link
Member

malter commented Apr 28, 2020

It would be possible to define the type as mutable but then the const call would change the behavior of the ConfigMap which infect is not a const behavior. ;-)
The better solution would be to define a const function that doesn't affect the type but returns the value if possible.

@hwiedPro
Copy link
Author

hwiedPro commented May 23, 2023

The problem persists. Mutable is not a valid approach. Please see the following example:

TEST_CASE("ConfigMap", "boolean")
{
    ConfigMap map;
    map["some_bool_value"] = true; 
    // The following line will fail during compilation, due to ambiguous overload for ‘operator==’ (operand types are ‘configmaps::ConfigItem’ and ‘bool’)
    // REQUIRE(map["some_bool_value"] == true);
    REQUIRE((bool)map["some_bool_value"] == true);
    std::cout << map.toJsonString()<< std::endl;
    /* This will output:
    {
        "some_bool_value": true    
    }
    */

    ConfigMap recovered = ConfigMap::fromJsonString(map.toJsonString());
    std::cout << recovered.toJsonString() << std::endl;
    /* No suddenly our bool value became a string
    This will output:
    {
        "some_bool_value": "true"    
    }
    */

    REQUIRE((bool)recovered["some_bool_value"] == true);
    std::cout << recovered.toJsonString() << std::endl;
    /* This will output again:
    {
        "some_bool_value": true    
    }
    */
}

This means that an entry that is JSON valid boolean is parsed/written as a String. And the file may be changed secretly in the background even when a should be const operation is performed.

BTW: The normal C++ conversion (bool) of an integer fails in ConfigMaps eventhough one could assume that this should work.

@jliersch
Copy link
Contributor

jliersch commented Jul 29, 2024

As ConfigMap inherits from std::map (this may be dangerous btw.), you can use at instead of operator[] potentially followed by toString. This should ideally be preceeded with a call to hasKey. The latter currently is not const, but I will create a pull request.

edit: #21

edit2: After some more testing I realized that just using std::map::at will not work. Sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants