refactor(libsinsp): manage memory in ast/parser via unique_ptrs

[LucaGuerra]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area libsinsp /area tests

Does this PR require a change in the driver versions?

What this PR does / why we need it:

This refactor is also the larger part of the effort to make our tests pass under AddressSanitizer.

All the ast::expr classes were holding pointers or vectors of pointers allocated manually via new and removed via delete, sometimes through virtual destructors. This should be fine in the happy path where everything works as planned, but becomes an issue where any unexpected behavior happens, such as any throw in the parsers. In that case, any allocated memory would not be freed and thus leaked. The issue is apparent in tests that exercise parsing errors or other edge cases when using LeakSanitizer which will loudly complain about a lot of leaked memory. This PR aims to remove any manual memory management in those classes and replace it with std::unique_ptrs.

I decided to solve the issue this way because any expression node owns the nodes below it, and the parser is recursive, normally operating by allocating the lower nodes and then assigning them to the nodes one level up. This also has the added benefit of removing virtual destructors since we don't need them anymore.

The contract with functions that return or get parameters as std::unique_ptr works as follows:

• If the function/constructor requires a std::unique_ptr it means that it's going to acquire full ownership of that pointer, which of course needs to be std::moved there.
• If the function returns a std::unique_ptr it means that it will give ownership of the object to the caller
• Same goes for std::vector<std::unique_ptr<>>

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

See inline comments.

Does this PR introduce a user-facing change?:

NONE

[FedeDP]

/cc @jasondellaluce

[poiana]

LGTM label has been added.

Git tree hash: a7a91d6c927305c33a0e98e3b04d2bedc4c04651

[LucaGuerra]

Addressed comment, plus another small thing that surfaced from myself getting confused by my own refactoring: added an explicit const when using a std::vector<std::unique_ptr> in a read only way so it is clear that the specific function is not transferring ownership (it could if it was non-const)

[poiana]

LGTM label has been added.

Git tree hash: 247ba8d0cded094647f5682458e039156dc52ca7

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [FedeDP,LucaGuerra]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment