On Wed, Mar 31, 2021 at 10:42 AM Alex White <alex.wh...@oarcorp.com> wrote: > > On Wed, Mar 31, 2021 at 11:22 AM Gedare Bloom <ged...@rtems.org> wrote: > > > > On Wed, Mar 31, 2021 at 10:05 AM Alex White <alex.wh...@oarcorp.com> wrote: > > > > > > The `rangeIndex` variable is 1 higher than the index at which the first > > > instruction address was found. This fixes the lookup after the loop to > > > account for that fact. > > > --- > > > tester/covoar/ObjdumpProcessor.cc | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tester/covoar/ObjdumpProcessor.cc > > > b/tester/covoar/ObjdumpProcessor.cc > > > index 62a06c5..1cfa877 100644 > > > --- a/tester/covoar/ObjdumpProcessor.cc > > > +++ b/tester/covoar/ObjdumpProcessor.cc > > > @@ -60,7 +60,7 @@ namespace Coverage { > > > lowAddress = coverageMap.getLowAddressOfRange(rangeIndex); > > > } > > > > > > - uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex); > > > + uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex - > > > 1); > > > > I guess this is because of the for loop. Maybe, you should prefer to > > exit the for loop with the proper index? > > That is what I had before this code was initially reviewed: > > for (rangeIndex = 0; ; rangeIndex++) { > lowAddress = coverageMap.getLowAddressOfRange(rangeIndex); > > if (firstInstructionAddress == lowAddress) { > break; > } > } > > The suggestion was made to move the condition into the loop rather than > use `break`: > > for (rangeIndex = 0; > firstInstructionAddress != lowAddress; > rangeIndex++) { > lowAddress = coverageMap.getLowAddressOfRange(rangeIndex); > } > > I did that without realizing it wasn't equivalent and didn't run enough > tests to catch it. > > Am I missing something here? Could I go back to my first solution? Is > there a more elegant way? >
Yes, since the first iteration will always happen, and you want to end with the 'right' value in the iterator, a do-while loop is more appropriate. > Your most tedious code reviewee, > > Alex > > :) > > > > > Fixing an off-by-1 bug by subtracting one from the point of > > consumption is a good way to ensure the bug occurs again if anyone > > reuses the rangeIndex variable later. > > That is true. > > > > > > uint32_t size = sizeWithoutNops; > > > uint32_t highAddress = lowAddress + size - 1; > > > uint32_t computedHighAddress = highAddress; > > > -- > > > 2.27.0 > > > > > > _______________________________________________ > > > devel mailing list > > > devel@rtems.org > > > http://lists.rtems.org/mailman/listinfo/devel > > _______________________________________________ > > devel mailing list > > devel@rtems.org > > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel