Added abstract base class History, so StaticLengthHistory and…
Audited0b66a6941c00
Actions

Referenced Files

None

Subscribers

goetzinger

• juhasz

Description

Added abstract base class History, so StaticLengthHistory and DynamicLengthHistory can inherit from it

Details

Auditors

• juhasz
goetzinger

Provenance

• btutzer	Authored on May 15 2019, 2:10 PM
• btutzer	Pushed on May 15 2019, 2:10 PM

Parents

R20:ac04eeeb6325: Coding style fixes

Branches

Unknown

Tags

Unknown

Event Timeline

• btutzer committed R20:0b66a6941c00: Added abstract base class History, so StaticLengthHistory and… (authored by • btutzer).May 15 2019, 2:10 PM

In T130#2047, @juhasz wrote:

I agree.
Let's keep the possibility to have a lightweight but statically sized History with std::array and add a dynamic version with std::vector.

I suggest to keep History as an interface that declares the public functions of the current implementation. StaticHistory and DynamicHistory would be two implementations of that interface based on std:array and std::vector, respectively; and DynamicHistory adds the resize feature of course.

Please have a look. History is now an abstract class and StaticLengthHistory and DynamicLengthHistory inherit from it.
@juhasz this branch also fixes some cosmetics that clang-tidy complained about. If any of them were intentional please let me know and I'll revert them.

This commit now requires audit.May 15 2019, 2:17 PM

I had a quick look, and it seems OK except for my inline comment about History::~History().

I also wonder why History::lengthOfHistory and History::policyOfHistory? I think simply History::length() and History::policy() would be just as understandable and much less characters.

Thanks for the cosmetic fixes! There were nothing intentional about them. (Intentionally diverging from the coding standard would be done by explicitly disabling clang-tidy and/or clang-format, see Dev.rst on Coding Standards.)

/include/rosa/agent/History.hpp
47	This must be virtual for instances of derived classes to be destructed correctly via reference and pointer of History.

This commit now has outstanding concerns.May 15 2019, 4:40 PM

In R20:0b66a6941c008121ff2f8f0f65abe87b7f625dc7#8753, @juhasz wrote:

I also wonder why History::lengthOfHistory and History::policyOfHistory? I think simply History::length() and History::policy() would be just as understandable and much less characters.

I think I'll go for 'History::maxLength' instead of 'History::length'. Otherwise one might confuse it with the current length ('History::numberOfEntries').

Thanks for the cosmetic fixes! There were nothing intentional about them. (Intentionally diverging from the coding standard would be done by explicitly disabling clang-tidy and/or clang-format, see Dev.rst on Coding Standards.)

Great!

• btutzer mentioned this in R20:383554aef975: Destructor of base class needs to be virtual. Shorter names for History::length….May 16 2019, 7:38 AM

In R20:0b66a6941c008121ff2f8f0f65abe87b7f625dc7#8753, @juhasz wrote:

I had a quick look, and it seems OK except for my inline comment about History::~History().

I also wonder why History::lengthOfHistory and History::policyOfHistory? I think simply History::length() and History::policy() would be just as understandable and much less characters.