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

More ref args #504

Merged
merged 1 commit into from Feb 26, 2015
Merged

More ref args #504

merged 1 commit into from Feb 26, 2015

Conversation

mikekaganski
Copy link
Contributor

No description provided.

{
return get_double(s);
};
const std::vector<double>& get_double_vector(std::string s)
const std::vector<double>& get_double_vector(const std::string &s) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the const on the right not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rightmost const makes this function "constant function": it means that

  1. we declare that the function does not change the state of the object.
  2. thus, compiler does not allow us to change any of object's members, and only allows to call their "const" functions;

and only this allows to call this function, when the object itself is const.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another C++ lesson for me!

On Thu, Feb 26, 2015 at 1:54 PM, mikekaganski notifications@github.com
wrote:

In include/CoolPropTools.h
#504 (comment):

     {
         return get_double(s);
     };
  •    const std::vector<double>& get_double_vector(std::string s)
    
  •    const std::vector<double>& get_double_vector(const std::string &s) const
    

the rightmost const makes this function "constant function": it means that

  1. we declare that the function does not change the state of the object.
  2. thus, compiler does not allow us to change any of object's members, and
    only allows to call their "const" functions;

and only this allows to call this function, when the object itself is
const.


Reply to this email directly or view it on GitHub
https://github.com/CoolProp/CoolProp/pull/504/files#r25463186.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is an impact on the optimization possible since it is very const?

On Thu, Feb 26, 2015 at 1:54 PM, Ian Bell ian.h.bell@gmail.com wrote:

Another C++ lesson for me!

On Thu, Feb 26, 2015 at 1:54 PM, mikekaganski notifications@github.com
wrote:

In include/CoolPropTools.h
#504 (comment):

     {
         return get_double(s);
     };
  •    const std::vector<double>& get_double_vector(std::string s)
    
  •    const std::vector<double>& get_double_vector(const std::string &s) const
    

the rightmost const makes this function "constant function": it means that

  1. we declare that the function does not change the state of the object.
  2. thus, compiler does not allow us to change any of object's members,
    and only allows to call their "const" functions;

and only this allows to call this function, when the object itself is
const.


Reply to this email directly or view it on GitHub
https://github.com/CoolProp/CoolProp/pull/504/files#r25463186.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and it was three-fold here.

  1. I noticed the param-by-value Dictionary (ReducingFunctions.h line 327). The dictionary has four maps, that contain numbers, strings, and vectors of numbers and strings. Each has to be recursively copied, and released after use, and each object has its constructors and destructors. So, naturally, I wanted to pass it by const ref. But when I did that, the error appeared that the object cannot be const, as the methods called are not const.
  2. I see that methods should not make changes to Dictionary - they are simple selectors. So I go and change their declarations as const, too. But compiler complains that as the method is const, it's illegal to call non-const methods of members there. I see, that the methods are implemented this way: first, the key is searched using find(), and only if found, the operator [] is called, that makes search again. This is not optimal per se, but additionally, in maps, operator [] is not const: it looks for the key, and if not found, it automatically adds new entry for this key, thus changing the object. This helped me to notice this inefficient implementation. So, I simplified things by making only one lookup using const method find().
  3. Further, it may help compiler generate more efficient code internally, even in places where the code is unchanged, as now it has a hint that the object's state stays unchanged.

@ibell
Copy link
Contributor

ibell commented Feb 26, 2015

Thanks for your work on CoolProp tidying up

ibell added a commit that referenced this pull request Feb 26, 2015
@ibell ibell merged commit 1dafe24 into CoolProp:master Feb 26, 2015
@mikekaganski
Copy link
Contributor Author

Actually, it's honour for me to help people that give their time, knowledge and work to others. Ant who do it for free! Thank you.

@ibell ibell added this to the v5.0.8 milestone Mar 1, 2015
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

Successfully merging this pull request may close these issues.

None yet

2 participants