Replace owning raw pointers with unique_ptr by reuk · Pull Request #1029 · diffblue/cbmc

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

 #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

 #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

  {
-    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

  {
-    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

      }

-      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

+      auto language=get_language_from_filename(filename);

-      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

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

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


if(!ptr) to be consistent

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

src/langapi/language_util.cpp

    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

+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

    }

-    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

+  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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

janmroczkowski Jun 21, 2017 •

edited

Loading

peterschrammel commented Jun 25, 2017 •

edited

Loading