Document remove_virtual_functions [DOC-85] #2786

LAJW · 2018-08-21T15:35:31Z

No description provided.

jeannielynnmoulton

Documentation seems clear, but I think you were supposed to write a summary in the main markdown file.

jeannielynnmoulton · 2018-08-21T16:15:17Z

src/goto-programs/remove_virtual_functions.cpp

+/// and the instance type or if fallback_action is set to
+/// ASSUME_FALSE, then function is substituted with a call to ASSUME(false)
+/// \symbol_table: Self explanatory
+/// \param goto_program [in/out]: GoTo program to modify


consider consistency in capitalisation of gOTo throughout

jeannielynnmoulton · 2018-08-21T16:15:38Z

src/goto-programs/remove_virtual_functions.cpp

 void remove_virtual_functions(goto_modelt &goto_model)
 {
  remove_virtual_functions(
    goto_model.symbol_table, goto_model.goto_functions);
 }

+/// Remove virtual function calls from the specified model


model function

jeannielynnmoulton · 2018-08-21T16:16:09Z

src/goto-programs/remove_virtual_functions.cpp

@@ -479,6 +506,9 @@ exprt remove_virtual_functionst::get_method(
  return symbol->symbol_expr();
 }

+/// Remove all virtual function calls in a goto program and replace
+/// them with calls to their most derived implementations
+/// Returns true if at least one function has been replaced


use \return

If I add \return this no longer is considered as \brief section, and linter is going to kill me.

Also I really, really don't like the pattern where a function that just returns something has to have its behaviour specified in the \return clause only. I don't believe any commercial library does that (except Java, where everything is void-returning).

If I add \return this no longer is considered as \brief section, and linter is going to kill me.

Is this just because of the missing full stop after "derived implementations?" Not a doxygen expert here, but AFAIK the first "." (if any) is interpreted by doxygen.

Everything before the first tag is considered as \brief (the high-level function description, it can be as long as you like). Yes, I'm missing full stop, thanks for pointing that out 👍

jeannielynnmoulton · 2018-08-21T16:16:59Z

src/goto-programs/remove_virtual_functions.cpp

+/// implementation. If there's a type mismatch between implementation
+/// and the instance type or if fallback_action is set to
+/// ASSUME_FALSE, then function is substituted with a call to ASSUME(false)
+/// \param goto_program [in/out]: GoTo program to modify


consider consistency in capitalisation of gOTo throughout

jeannielynnmoulton · 2018-08-21T16:17:45Z

src/goto-programs/remove_virtual_functions.cpp

@@ -72,6 +72,13 @@ remove_virtual_functionst::remove_virtual_functionst(
 {
 }

+/// Replace specified virtual function call with a static call to its
+/// last implementation
+/// \param goto_program [in/out]: GoTo program to modify


consider consistency in capitalisation of gOTo throughout

I think it is normally capitalised as GOTO

thk123

As Jeanie pointed out needs overview in goto-programs/README.md in a section called Remove Virtual Functions

thk123 · 2018-08-21T16:42:10Z

src/goto-programs/remove_virtual_functions.cpp

@@ -72,6 +72,13 @@ remove_virtual_functionst::remove_virtual_functionst(
 {
 }

+/// Replace specified virtual function call with a static call to its
+/// last implementation
+/// \param goto_program [in/out]: GoTo program to modify


I think it is normally capitalised as GOTO

thk123 · 2018-08-21T17:03:15Z

src/goto-programs/remove_virtual_functions.cpp

@@ -72,6 +72,13 @@ remove_virtual_functionst::remove_virtual_functionst(
 {
 }

+/// Replace specified virtual function call with a static call to its
+/// last implementation


What does implementation mean?

thk123 · 2018-08-21T17:13:02Z

src/goto-programs/remove_virtual_functions.cpp

 void remove_virtual_functions(goto_modelt &goto_model)
 {
  remove_virtual_functions(
    goto_model.symbol_table, goto_model.goto_functions);
 }

+/// Remove virtual function calls from the specified model


You can use \copydoc remove_virtual_functions rather than actually copy-pasting the docs

I think I can't do that because the function differs a bit (it's not a method, takes in more arguments).

peterschrammel · 2018-08-21T17:37:40Z

src/goto-programs/remove_virtual_functions.cpp

+/// implementation. If there's a type mismatch between implementation
+/// and the instance type or if fallback_action is set to
+/// ASSUME_FALSE, then function is substituted with a call to ASSUME(false)
+/// \symbol_table: Self explanatory


Self explanatory -> Symbol table

LAJW · 2018-08-22T11:05:55Z

I've applied your suggestions, with one exception.

@thk123 There are no sections for any of the contents of the directory in there, unlike in java_bytecode, where yall been working. There's a really good high-level overview, and my file seems to be an implementation detail, rather than one of the entry points, so I'm not sure it should go in there.

thk123

For the overview I was imaging adding to that readme a

\section goto-programs-lowerings GOTO program lowers

_Summary of lowering as a concept_
_List of lowerings_

\subsection goto-programs-remove-virtual-function remove_virtual_functions

\ref remove_virtual_functions replaces virtual function calls with a switch on the class identifier with the possible implementations of that method. The virtual call is therefore replaced by a collection of static calls.

But no reason to hold up this PR for that.

thk123 · 2018-08-24T10:00:04Z

Unfortunately the CI is failing due to two errors in this doxygen:

remove_virtual_functions.cpp:594: warning: argument 'target' of command @param is not found in the argument list of remove_virtual_function
remove_virtual_functions.cpp:594: warning: argument 'functions' of command @param is not found in the argument list of remove_virtual_function

peterschrammel · 2018-08-30T07:38:03Z

@LAJW, please fix @thk123's remaining comments.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: f14b163).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/93144212

LAJW · 2018-11-30T12:47:32Z

CI Fixed, comments addressed (separate ticket has been created)

LAJW requested review from thk123 and jeannielynnmoulton August 21, 2018 15:35

LAJW requested review from kroening, peterschrammel, pkesseli, smowton and tautschnig as code owners August 21, 2018 15:35

jeannielynnmoulton approved these changes Aug 21, 2018

View reviewed changes

thk123 reviewed Aug 21, 2018

View reviewed changes

peterschrammel added the documentation label Aug 21, 2018

peterschrammel reviewed Aug 21, 2018

View reviewed changes

peterschrammel assigned LAJW Aug 21, 2018

LAJW changed the title ~~Document remove_virtual_functions~~ [DOC-25] Document remove_virtual_functions Aug 22, 2018

thk123 approved these changes Aug 23, 2018

View reviewed changes

thk123 changed the title ~~[DOC-25] Document remove_virtual_functions~~ Document remove_virtual_functions [DOC-85] Oct 4, 2018

Document all functions

f14b163

allredj reviewed Nov 30, 2018

View reviewed changes

peterschrammel approved these changes Nov 30, 2018

View reviewed changes

peterschrammel merged commit 1eb1df6 into diffblue:develop Nov 30, 2018

Document remove_virtual_functions [DOC-85] #2786

Document remove_virtual_functions [DOC-85] #2786

Uh oh!

Conversation

LAJW commented Aug 21, 2018

Uh oh!

jeannielynnmoulton 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!

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!

LAJW commented Aug 22, 2018

Uh oh!

thk123 left a comment

Choose a reason for hiding this comment

Uh oh!

thk123 commented Aug 24, 2018

Uh oh!

peterschrammel commented Aug 30, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

LAJW commented Nov 30, 2018

Uh oh!

Uh oh!