Use irept instead of symbol_exprt in java_lambda_method_handlest [blocks: #2310]

[tautschnig]

A java_lambda_method_handlest really is just a collection of identifiers - there
is no need to put them into a symbol_exprt, instead the identifier can directly
be used as the id of an irept. This reduces the memory footprint, but more
importantly avoids a warning of using a deprecated symbol_exprt constructor.
This warning is well placed, because those symbol_exprt would never have a type
set, which makes them invalid symbol_exprt anyway.

• Each commit message has a non-empty body, explaining why the change was made.
• n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
• Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
• n/a My commit message includes data points confirming performance improvements (if claimed).
• My PR is restricted to a single feature or bugfix.
• n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

[allredj]

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 6d99326).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90526786
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

• the cbmc commit has disappeared in the mean time (e.g. in a force-push)
• the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

[allredj]

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

[allredj]

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 193bb15).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90579104
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

• the cbmc commit has disappeared in the mean time (e.g. in a force-push)
• the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

[thk123]

On the face of it seems reasonable - but some thought went in to this so would like the time to properly consider and check with TG as I don't recall why we opted for symbol_exprt here

[tautschnig]

@thk123 +1 for careful evaluation with TG, please do re-assign as appropriate.

[allredj]

⚠️
Diffblue compatibility check is currently unavailable.
Please create manual bump.
(cbmc commit: 592db89).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90843000

[thk123]

So this is fine - I think we will revisit this - this is as far as lambdas get at the moment (i.e. they still don't work) - it might be it makes sense to put a bespoke

class lambda_method_handlet : public irept
{
  irep_idt lambda_identifier() const;
  code_tyept lambda_type() const;
  // lambda type?
}

but lets wait until we know what we need rather than second guessing it.

The only thing I might suggest now is typedef lamdba_method_handle_identifiert as an irept so more obvious that it isn't a tree of stuff, just a name. In fact, would:

class lambda_method_handle_identifiert : private irept
{
public:
  explicit lambda_method_handle_identifiert(const irep_idt &identifier) : irept(identifier)
  lambda_identifier() const { return id(); }
}

be a good immediate solution? (i.e. using private inheritance to essentially make this behave like an irep_idt)

Anyway - it's fine as it is until we know what we do with the data next.

[thk123]

I made a manual bump but I don't expect it to break anything (and indeed Joel-bot linked to a passing build) https://github.com/diffblue/test-gen/pull/2458

[tautschnig]

Why would you suggest private inheritance here? I think the one with public inheritance would work fine, though. Just not entirely sure whether it's worth the effort if you are about to rewrite some of this anyway?

[thk123]

Well "about to" is probably pushing it! Happy for it to go in as is 🙂

I'm not sure about private inheritance - there is nothing specific to this case that means it should be used over any other irep-inheriting class, but I think it might be a good idea:

AFAICT it would forbid direct irep manipulation (a major source of bugs/refactoring pain when people just add random sub trees and extra data inconsistently) to make them behave more like properly typed C++ classes, but wouldn't have any performance impact or require any major restructuring (only need to remove direct irep access - but these probably should be removed anyway).

[thk123]

TG bump has passed

[tautschnig]

@thk123 I've gone the coward's route and only included your suggestion as a comment rather than changing the code at this point. I hope that's ok?

[allredj]

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 49d4914).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90890870
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

• the cbmc commit has disappeared in the mean time (e.g. in a force-push)
• the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

[allredj]

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