Opened 3 years ago

Last modified 3 years ago

#5013 closed change

Improve const-correctness in libadblockplus — at Version 6

Reported by: sergz Assignee: hfiguiere
Priority: P4 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, hfiguiere, eric@…, oleksandr Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29391775/
https://codereview.adblockplus.org/29393589/
https://codereview.adblockplus.org/29394609/
https://codereview.adblockplus.org/29408747/
https://codereview.adblockplus.org/29410664/

Description

Background

We have some non-const methods which don't change the logical state and I would like to propose to make them const because I find it good for the user of the library from the design point of view.

Dangerous formal bitwise correctness

However, I would like to pay attention to one concern. Usually it's fine for me when a const method has to explicitly modify a state because of some legit reason (like lazy or caching calculations, though it's not our case) but I would like to minimize and limit cases when within a const method we copy a const pointer to a non-const pointer and call non-const methods because I find it error-prone because it's easy to not spot it, it can very quickly get a huge scale and compiler won't help if there are some future controversial changes.
Unfortunately there is plenty of such cases, for example, we often call a non-const methods of jsEngine member from a potentially const functions, it is the above case because std::shared_ptr<T>::operator->() const returns a non-const pointer and we can modify the physical state. We actually even cannot mark some methods of JsEngine as constant methods because of legit reason because it makes calls to JavaScript world which does not have a concept of const-correctness in terms of C++.
In order to figure it out I would like to propose to allow such cases only withing Js* classes, consider to have const versions of methods of Js* classes for logically const calls and get rid of members allowing the issue, such as std::shared_ptr<JsXXX>, in non-Js* classes.
Your opinion?

What to change

Fill it later, let's firstly evaluate the issue, then discuss the way we do it and afterwards it will be possible to write down the desired change.

Change History (6)

comment:1 Changed 3 years ago by hfiguiere

  • Review URL(s) modified (diff)

comment:2 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 5013 - Use auto and range iterator when appropriate.

comment:3 Changed 3 years ago by hfiguiere

  • Owner set to hfiguiere

comment:4 Changed 3 years ago by hfiguiere

  • Review URL(s) modified (diff)

comment:5 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 5013 - Make more methods const.

comment:6 Changed 3 years ago by hfiguiere

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.