Replace owning raw pointers with unique_ptr #1029

reuk · 2017-06-17T22:08:18Z

The same as #835 but targeting test-gen-support.

smowton · 2017-06-19T09:15:32Z

Suggest whoever comes to merge this should instead merge in the corresponding master commit and then apply residual changes in the merge, such that the commit graph resembles the true situation and merge decisions are more likely to be taken automatically in the future
Suggest unrelated virtual -> override changes be done in a different PR / commit
I don't understand the purpose of std::forward in freert, could you explain?

thk123

I'm neither well versed enough in C++ memory types or the code in question to approve but I've spotted some inconsistencies in the changes so I've pointed them out

thk123 · 2017-06-19T10:00:53Z

src/analyses/ai.h

@@ -18,6 +18,7 @@ Author: Daniel Kroening, [email protected]
 #include <util/json.h>
 #include <util/xml.h>
 #include <util/expr.h>
+#include <util/make_unique.h>


Don't we need #include <memory>

util/make_unique.h itself includes memory. My preference would be to leave it as-is, and then if we change to C++14 we can find-and-replace util/make_unique -> memory and util_make_unique -> std::make_unique.

thk123 · 2017-06-19T10:01:02Z

src/analyses/goto_rw.cpp

@@ -19,6 +19,7 @@ Date: April 2010
 #include <util/endianness_map.h>
 #include <util/arith_tools.h>
 #include <util/simplify_expr.h>
+#include <util/make_unique.h>


Don't we need #include <memory>

thk123 · 2017-06-19T10:02:40Z

src/analyses/goto_rw.h

@@ -108,8 +109,8 @@ class rw_range_sett

  const range_domaint &get_ranges(objectst::const_iterator it) const
  {
-    assert(dynamic_cast<range_domaint*>(it->second)!=0);
-    return *static_cast<range_domaint*>(it->second);
+    assert(dynamic_cast<range_domaint*>(it->second.get()));


While you're modifying the line, can this become INVARIANT or possibly one of the others?

I'm not against this, but wonder whether it should really be part of this PR.

@reuk Yup you're right - it is already a big change

thk123 · 2017-06-19T10:02:47Z

src/analyses/goto_rw.h

@@ -277,8 +278,8 @@ class rw_guarded_range_set_value_sett:public rw_range_set_value_sett

  const guarded_range_domaint &get_ranges(objectst::const_iterator it) const
  {
-    assert(dynamic_cast<guarded_range_domaint*>(it->second)!=0);
-    return *static_cast<guarded_range_domaint*>(it->second);
+    assert(dynamic_cast<guarded_range_domaint*>(it->second.get())!=nullptr);


While you're modifying the line, can this become INVARIANT or possibly one of the others?

thk123 · 2017-06-19T10:04:09Z

src/analyses/reaching_definitions.h

-  value_setst * value_sets;
-  is_threadedt * is_threaded;
-  dirtyt * is_dirty;
+  std::unique_ptr<value_setst> value_sets;


Do we need a #include <memory>

thk123 · 2017-06-19T10:09:21Z

src/cbmc/cbmc_solvers.cpp

-
-  // We offer the option to disable the SAT preprocessor
-  if(options.get_bool_option("sat-preprocessor"))
+  auto prop=[this]() -> std::unique_ptr<propt>


Why is this a lambda?

So that prop can be directly initialised to its correct state rather than two-stage constructed-then-initialised. I think this is cleaner than

std::unique_ptr<propt> prop; // We offer the option to disable the SAT preprocessor if(options.get_bool_option("sat-preprocessor")) { no_beautification(); prop=util_make_unique<satcheckt>(); } else prop=util_make_unique<satcheck_no_simplifiert>();

...although there's not much in it, so if you'd prefer it like that then I'll change it.

Don't have a strong preference - do have a strong preference that you drop the auto if you do it this way, I assumed on first reading that prop was a lambda

thk123 · 2017-06-19T10:10:07Z

src/clobber/clobber_parse_options.cpp

@@ -247,9 +247,9 @@ bool clobber_parse_optionst::get_goto_program(
        return true;
      }

-      languaget *language=get_language_from_filename(filename);
+      auto language=get_language_from_filename(filename);


Type not on RHS so don't think this should be auto

thk123 · 2017-06-19T10:10:44Z

src/clobber/clobber_parse_options.cpp


-      if(language==NULL)
+      if(language==nullptr)


if(!language) to be consistent with earlier changes

Well spotted :)

thk123 · 2017-06-19T10:13:47Z

src/langapi/language_util.cpp


-  if(ptr==NULL)
+  if(ptr==nullptr)


if(!ptr) to be consistent

thk123 · 2017-06-19T10:13:51Z

src/langapi/language_util.cpp

@@ -28,9 +28,9 @@ static languaget* get_language(
     symbol->mode=="")
    return get_default_language();

-  languaget *ptr=get_language_from_mode(symbol->mode);
+  auto ptr=get_language_from_mode(symbol->mode);


Type not on RHS so don't think this should be auto

reuk · 2017-06-19T10:40:35Z

@smowton:

Good idea, but that PR has been open two months and no-one is doing anything about it. This stuff is important, so the plan was to merge it into test-gen-support, which is normally quicker.
Good point, removed from this PR.
For wrappers like this which don't implement any semantic changes, I like to use forward so that the interface for the wrapper exactly matches the interface of the wrapped function.

smowton · 2017-06-19T10:56:31Z

Let's try to aggrevate for the master PR tomorrow in the interest of less merge chaff downstream

reuk · 2017-06-19T13:25:20Z

Updated to include <memory> as requested.

reuk · 2017-06-20T09:32:44Z

Should I apply the changes from this PR to the original one?

janmroczkowski · 2017-06-20T09:37:22Z

src/util/freer.h

@@ -12,7 +12,7 @@ Author: Reuben Thomas, [email protected]
 #include <cstdlib>
 #include <utility>

-struct freert final
+struct freert
 {
  template <typename T>
  void operator()(T &&t) const


You can still have final on member function.

It's not a virtual method, therefore putting final on it causes a compiler error.

Fine with me, I was not really a fan of putting final in there in the first place.

thk123 · 2017-06-20T10:07:07Z

@reuk I suppose so (sorry for not reviewing the original one - that would have been more helpful)

peterschrammel · 2017-06-20T10:58:09Z

src/cbmc/cbmc_parse_options.cpp

@@ -750,19 +750,17 @@ void cbmc_parse_optionst::preprocessing()
      return;
    }

-    languaget *ptr=get_language_from_filename(filename);
+    std::unique_ptr<languaget> ptr=get_language_from_filename(filename);


I'd prefer calling it language instead of ptr

janmroczkowski · 2017-06-21T12:40:07Z

src/util/unicode.cpp


-  // the following never gets deleted
-  const char **argv_narrow=new const char *[argc+1];
+  std::vector<const char *> argv_narrow(argc+1);
  argv_narrow[argc]=0;

  for(int i=0; i<argc; i++)


While the above fixes array leak, it still leaks elements.
Maybe change from char* to std::string or unique pointer would help.

peterschrammel · 2017-06-25T15:58:55Z

Let's get #835 merged into master first.

reuk force-pushed the no-raw-ptrs-tgs branch 5 times, most recently from a159f00 to 42a2bf5 Compare June 18, 2017 09:10

Replace owning raw pointers with unique_ptr

513b705

reuk force-pushed the no-raw-ptrs-tgs branch from 42a2bf5 to 513b705 Compare June 18, 2017 14:00

thk123 reviewed Jun 19, 2017

View reviewed changes

Revert virtual/override changes

3b56b1b

Apply @thk123's fixes

1a6249c

reuk force-pushed the no-raw-ptrs-tgs branch from caee053 to 1a6249c Compare June 19, 2017 10:56

Fix includes

30d8fb5

Remove "final" from freer struct to reduce unique_ptr size

9688d11

janmroczkowski reviewed Jun 20, 2017

View reviewed changes

reuk mentioned this pull request Jun 20, 2017

Replace owning raw pointers #835

Merged

peterschrammel reviewed Jun 20, 2017

View reviewed changes

Rename ptr -> language

59169be

janmroczkowski reviewed Jun 21, 2017

View reviewed changes

reuk added 2 commits June 21, 2017 13:49

Implement @tautschnig's fixes

f1ae9fe

Fix residual memory leak from narrow_argv

92e02b5

reuk force-pushed the no-raw-ptrs-tgs branch from 80f7b89 to 92e02b5 Compare June 21, 2017 13:06

reuk closed this Jul 10, 2017

Replace owning raw pointers with unique_ptr #1029

Replace owning raw pointers with unique_ptr #1029

Uh oh!

Conversation

reuk commented Jun 17, 2017

Uh oh!

smowton commented Jun 19, 2017

Uh oh!

thk123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reuk commented Jun 19, 2017

Uh oh!

smowton commented Jun 19, 2017

Uh oh!

reuk commented Jun 19, 2017

Uh oh!

reuk commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thk123 commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janmroczkowski Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterschrammel commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

janmroczkowski Jun 21, 2017 •

edited

Loading

peterschrammel commented Jun 25, 2017 •

edited

Loading