Opened on 03/17/2017 at 10:05:17 PM
Closed on 04/19/2017 at 11:39:51 AM
#5013 closed change (fixed)
Improve const-correctness in libadblockplus
| 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/ |
||
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.
Attachments (0)
Change History (12)
comment:2 Changed on 03/23/2017 at 01:18:38 PM by abpbot
comment:3 Changed on 03/24/2017 at 02:21:39 PM by hfiguiere
- Owner set to hfiguiere
comment:5 Changed on 03/24/2017 at 05:06:07 PM by abpbot
A commit referencing this issue has landed:
Issue 5013 - Make more methods const.
comment:7 Changed on 03/24/2017 at 07:26:02 PM by abpbot
A commit referencing this issue has landed:
Issue 5013 - bustage: use the proper type for callbackArgs.
comment:8 Changed on 04/10/2017 at 03:09:53 PM by hfiguiere
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:9 Changed on 04/11/2017 at 11:58:30 AM by abpbot
A commit referencing this issue has landed:
Issue 5013 - Mark more method as const
comment:10 Changed on 04/12/2017 at 03:29:23 PM by hfiguiere
- Review URL(s) modified (diff)
comment:11 Changed on 04/19/2017 at 11:38:37 AM by abpbot
Some commits referencing this issue have landed:
comment:12 Changed on 04/19/2017 at 11:39:51 AM by hfiguiere
- Resolution set to fixed
- Status changed from reviewing to closed
A commit referencing this issue has landed:
Issue 5013 - Use auto and range iterator when appropriate.