[MonitorsLib 0/1] Request for peer-review

[Edoardo Paone]

I think there is a small bug in the index used for the list of Operating Points.

Edoardo Paone (1):
[AS-RTM] Bugfix in OP index

rtlib/monitors/op_manager.cc | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

--
1.7.9.5

[Edoardo Paone]

Signed-off-by: Edoardo Paone <pa...@elet.polimi.it>
---

rtlib/monitors/op_manager.cc | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/rtlib/monitors/op_manager.cc b/rtlib/monitors/op_manager.cc
index 95f19e8..3df9026 100644
--- a/rtlib/monitors/op_manager.cc
+++ b/rtlib/monitors/op_manager.cc
@@ -61,6 +61,7 @@ bool OPManager::getHigherOP(OperatingPoint &op) {
if (vectorId == 0)
return false;

+ vectorId--;
op = operatingPoints[vectorId];
return true;
}
@@ -109,7 +110,10 @@ bool OPManager::getLowerOP(OperatingPoint &op, const OPFilterList &opFilters) {
}

bool OPManager::getHigherOP(OperatingPoint &op, const OPFilterList &opFilters) {
- for (int id = vectorId-1;id > 0; --id){
+ if (vectorId == 0)
+ return false;
+
+ for (int id = vectorId-1; id >= 0; --id){
if (isValidOP(operatingPoints[id], opFilters)){
vectorId = id;
op = operatingPoints[id];
--
1.7.9.5

[Patrick Bellasi]

Hi Edoardo,

   could you provide a commit which better describe the bugfix, so that I can merge it to mainline.

Ciao Patrick

--
1.7.9.5

--
You received this message because you are subscribed to the Google Groups "BOSP Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bosp-devel+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--
#include <best/regards.h>

Patrick Bellasi
Post-Doc at Politecnico di Milano
http://home.dei.polimi.it/bellasi

[Vincenzo Consales]

Hello Edoardo,

I had a look at the source code and yes, I agree with you: there is a bug in OPManager::getHigherOP(OperatingPoint &op). Thanks to have spotted it out and to have proposed the fix. For the second one (OPManager::getHigherOP(OperatingPoint &op, const OPFilterList &opFilters)), the if clause  you have added is redundant because if vectorId is zero then the for loop is not executed. Anyway, for readability sake, I guess we can also change that piece of code.

Thanks and best regards,

Vincenzo

2013/7/1 Edoardo Paone <edoa...@gmail.com>

--
1.7.9.5

[Edoardo Paone]

Hi Vincenzo,

Thanks for your reply, I really appreciate your review.

Regarding the method OPManager::getHigherOP(OperatingPoint &op, const OPFilterList &opFilters), my concern was actually on the return value. If I do not include the value 0 in the for loop, the test isValidOP() on vectorId 0 is never done, so the method returns false even if the OP with vectorId 0 is a valid one (i.e. it meets the requirements).

For example, if the current OP is 5, and the OP 0 allows to meet the constraint (it is valid), I expected the above method to return true when the for loop reaches vectorId 0. As it was before, the OP 0 could be selected only if the method getHigherOP() is called by getNextOP() -> getCurrentOP(), which always starts from vectorId 0 to search the nextOP.

Please correct if I'm wrong, you know the MonitorsLib much better than me :)

Cheers,

Edoardo

[Vincenzo Consales]

Hi Edoardo,

thanks to you to use and test the lib!

About your last comment you are definitely right, my bad! Looking at the diff I did not notice the change from > to >= and that's why I said the code was the same!

Anyway, the if clause is still redundant but I agree about the for loop.

Btw, we should think about changing the naming of these functions. After some time without seeing them, I had to spend a couple of minutes to remember the meaning of Higher and Lower. Maybe we could call them Better and Worse or Better and Degraded (like getBetterOP, getWorseOP for example).

Bye,

Vincenzo

2013/7/10 Edoardo Paone <pa...@elet.polimi.it>

[Edoardo Paone]

You're right, the if clause is redundant, I'll remove it.

Regarding the re-naming of methods, I'll take it into account in case we will go through a major revision.

Thank you again,

Edoardo