Task #1531: Change the return type of lookup method - NLSR - NDN project issue tracking system

Actions

Copy link

Task #1531

closed

Change the return type of lookup method

Added by Yingdi Yu over 11 years ago. Updated over 11 years ago.

Status:

Closed

Priority:

High

Assignee:

A K M Mahmudul Hoque

Target version:

Start date:

04/23/2014

Due date:

% Done:

100%

Estimated time:

Description

For methods like this:

std::pair<NameLsa&, bool>
Lsdb::getNameLsa(string key)
{
  std::list<NameLsa>::iterator it = std::find_if(m_nameLsdb.begin(),
                                                 m_nameLsdb.end(),
                                                 bind(nameLsaCompareByKey, _1, key));
  if (it != m_nameLsdb.end())
  {
    return std::make_pair(boost::ref((*it)), true);
  }
  NameLsa nlsa;
  return std::make_pair(boost::ref(nlsa), false);
}

We do not need to return std::pair. The purpose of the method is to lookup a NameLsa in Lsdb. It would be reasonable to return just a pointer to NameLsa or an empty pointer if the NameLsa does not exist.

Given this is a lookup method, it would be better to name it as findNameLsa rather than getNameLsa.

Also, the parameter key, as suggested by its name, should not be changed during the lookup process. In this case, please use const std::string&.

Similar issues appear in many places in the code, please fix them as well.

Actions

Copy link

Updated by A K M Mahmudul Hoque over 11 years ago

Status changed from New to In Progress

Its not just lookup. Thats why it is get. It looks up and return the LSA. So Update operation is done on the LSA, like changing sequence number when refreshing, updating lifetime etc. So the purpose of method is not just look up. So method name should remain same. Will change the return type and necessary modification.

Actions

Copy link

Updated by Alex Afanasyev over 11 years ago

Hmm. What update is done on LSA? From the code I see it just searches for the item...

Actions

Copy link

Updated by A K M Mahmudul Hoque over 11 years ago

in install(Adj/Cor/Name)Lsa methods update operation is performed on the returned reference.

Actions

Copy link

Updated by Alex Afanasyev over 11 years ago

But then how is it relevant to this method? In install you will find, update if there is an entry, insert if there is no entry, and proceed. I haven't look at the code, but if the desired behavior is to find existing item or insert new item, then you can rename it as "insert", keep pair, but instead of reference, return either pointer or iterator (with iterator, it would match STL style).

The reason the term "find" fits here better is that it is absolutely valid to return "none". Semantics of "get" is that it always return value and if it is impossible, it is an exceptional case and at least exception should be thrown.

Actions

Copy link

Updated by A K M Mahmudul Hoque over 11 years ago

Hmm. I see but I can return pair of pointer and bool right?

Actions

Copy link

Updated by Alex Afanasyev over 11 years ago

yes. pointer is about the same as iterator, so this should not cause any problem.

Actions

Copy link

Updated by Lan Wang over 11 years ago

I think the reason why Yingdi set up the task is that the new code does not compile on Ashlesh's machine. Here's Yingdi's suggestion " It is not a good practice to use reference in std::pair. It may or may not compile depending on the system. (http://stackoverflow.com/questions/3769781/stdpair-of-references)". And below is Ashlesh's errors:

On Apr 23, 2014, at 4:48 PM, "Ashlesh Gawande (agawande)" [email protected]
wrote:

I am using: (uname -n)

Linux ashlesh-VirtualBox 3.11.0-12-generic #19-Ubuntu SMP Wed Oct 9 16:20:46 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

(gcc -v)
gcc version is 4.8.1

From: Ashlesh Gawande (agawande)
Sent: Wednesday, April 23, 2014 4:44 PM
To: Lan Wang (lanwang); Yingdi Yu
Cc: A K M Mahmudul Hoque (ahoque1)
Subject: RE: unit tests

I am still testing with old code.

When I try to compile with the new code
I am getting the following error:
In file included from /usr/include/c++/4.8/bits/stl_algobase.h:64:0,
from /usr/include/c++/4.8/bits/char_traits.h:39,
from /usr/include/c++/4.8/ios:40,
from /usr/include/c++/4.8/ostream:38,
from /usr/include/c++/4.8/iostream:39,
from ../src/communication/interest-manager.cpp:1:
/usr/include/c++/4.8/bits/stl_pair.h: In instantiation of ‘struct std::pair’:
../src/communication/interest-manager.cpp:121:30: required from here
/usr/include/c++/4.8/bits/stl_pair.h:112:26: error: forming reference to reference type ‘nlsr::NameLsa&’
_GLIBCXX_CONSTEXPR pair(const _T1& __a, const _T2& __b)