generate-function-body: set function in source_location

[tautschnig]

This was set in only one of the three cases; also consistently use the prettiest
name available.

[peterschrammel]

Using display_name is a good idea, in principle, but using it only here will result in inconsistent output. More importantly, there are many usages of source_location.get_function() that expect it to return a symbol identifier. These would need to be fixed first.

[tautschnig]

That's scary - would you be able to point me to any of those get_function() uses so that I have a starting point?

[peterschrammel]

Most of the occurrences use the function for uniqueness of names, e.g.

cbmc/src/goto-programs/set_properties.cpp

Line 58 in 706e391

irep_idt function=it->source_location.get_function();

This seems unnecessary because we could take the function from the instruction (same for XML):

cbmc/src/goto-programs/json_goto_trace.cpp

Line 191 in 7f3ba30

if(ns.lookup(source_location.get_function(), function_name))

I doubt this would guarantee uniqueness of property names even at the moment unless pretty names are as unique as identifiers (this code is probably duplicated in many places):

cbmc/src/goto-programs/json_goto_trace.cpp

Line 47 in 7f3ba30

? id2string(step.pc->source_location.get_function()) + ".unwind." +

There are a couple of this sort that I am to blame for:

cbmc/src/ansi-c/c_nondet_symbol_factory.cpp

Line 39 in c6beb68

type, id2string(loc.get_function()), prefix, loc, ID_C, symbol_table);

...

[tautschnig]

I am now refraining from using the display_name. The PR is now all about just setting the function name.

[kroening]

The idea to use the result of source_locationt::get_function() as an identifier has a key problem that the linker doesn't know about these -- i.e., on symbol collision, you'll have the wrong identifier.

I am thus currently biased towards saying that source_locationt::get_function() is meant to be human-readable.

There could also be a case for removing it entirely, as the information is often available elsewhere; it's also not strictly a 'source location'.

[kroening]

The current view is to remove it entirely.

[codecov]

Codecov Report

Base: 78.34% // Head: 78.34% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (56fda10) compared to base (a53fa0f).
Patch coverage: 93.75% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2149   +/-   ##
========================================
  Coverage    78.34%   78.34%           
========================================
  Files         1644     1645    +1     
  Lines       190313   190357   +44     
========================================
+ Hits        149097   149132   +35     
- Misses       41216    41225    +9     

Impacted Files	Coverage Δ
jbmc/src/janalyzer/janalyzer_parse_options.cpp	48.58% <ø> (ø)
.../src/java_bytecode/character_refine_preprocess.cpp	54.27% <ø> (ø)
...mc/src/java_bytecode/character_refine_preprocess.h	100.00% <ø> (ø)
jbmc/src/java_bytecode/ci_lazy_methods.h	100.00% <ø> (ø)
jbmc/src/java_bytecode/ci_lazy_methods_needed.cpp	98.41% <ø> (ø)
jbmc/src/java_bytecode/ci_lazy_methods_needed.h	100.00% <ø> (ø)
jbmc/src/java_bytecode/code_with_references.cpp	84.61% <ø> (ø)
jbmc/src/java_bytecode/java_bmc_util.cpp	100.00% <ø> (ø)
...mc/src/java_bytecode/java_bytecode_convert_class.h	0.00% <ø> (ø)
...src/java_bytecode/java_bytecode_convert_method.cpp	97.69% <ø> (ø)
... and 413 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.