Re: [gold][patch] Add a plugin callback for setting the extra search path

[Cary Coutant <ccoutant at google dot com>]

> include/
> 2010-06-18 ?Rafael Espindola ?<espindola@google.com>
>
> ? ? ? ?* plugin-api.h (ld_plugin_set_extra_library_path): New.
> ? ? ? ?(ld_plugin_tag): Add LDPT_SET_EXTRA_LIBRARY_PATH.
> ? ? ? ?(ld_plugin_tv): Add tv_set_extra_library_path.
>
> gold/
> 2010-06-18 ?Rafael Espindola ?<espindola@google.com>
>
> ? ? ? ?* fileread.cc (Input_file::find_fie): New
> ? ? ? ?(Input_file::open): Use Input_file::find_fie.
> ? ? ? ?* fileread.h (Input_file::find_fie): New
> ? ? ? ?* plugin.cc (set_extra_library_path): New.
> ? ? ? ?(Plugin::load): Add set_extra_library_path to the transfer vector.
> ? ? ? ?(Plugin_manager::set_extra_library_path): New.
> ? ? ? ?(Plugin_manager::add_input_file): Use the extra search path if set.
> ? ? ? ?(set_extra_library_path(): New.
> ? ? ? ?* plugin.h (Plugin_manager): Adde set_extra_library_path and
> ? ? ? ?extra_search_path_.

s/Adde/Add/

-// Open the file.
-
-// If the filename is not absolute, we assume it is in the current
-// directory *except* when:
-//    A) input_argument_->is_lib() is true;
-//    B) input_argument_->is_searched_file() is true; or
-//    C) input_argument_->extra_search_path() is not empty.
-// In each, we look in extra_search_path + library_path to find
-// the file location, rather than the current directory.
-
 bool
-Input_file::open(const Dirsearch& dirpath, const Task* task, int *pindex)
+Input_file::find_file(const Dirsearch& dirpath, int *pindex,
std::string *namep)

find_file() needs a top comment, but I think the block of comments
under "Open the file" should go with the find_file() rather than
remain with open().

I'd suggest that you pass input_argument, is_in_sysroot, and
&found_name as additional parameters to find_file(), and that should
be sufficient to allow find_file() to live outside the Input_file
class.

While I'm not opposed to factoring find_file() out from open(), it's
not at all clear why you bothered -- there's still only the one call
to it, isn't there? Are you planning to make use of this routine in a
future patch?

+      if (this->input_argument_->extra_search_path())
+	{
+	  // First, check extra_search_path.
+	  std::string prefix = this->input_argument_->extra_search_path();
+	  if (!IS_DIR_SEPARATOR (prefix[prefix.length() - 1]))
+	    prefix += '/';
+
+	  name = prefix + n1;
+	  struct stat dummy_stat;
+	  if (*pindex <= 0 && ::stat(name.c_str(), &dummy_stat) >= 0)
+	    {
+	      this->found_name_ = this->input_argument_->name();
+	      *namep = name;
+	      return true;
+	    }
+
+	  name = prefix + n2;
+	  if (!n2.empty() && *pindex <= 0
+	      && ::stat(name.c_str(), &dummy_stat) >= 0)
+	    {
+	      this->found_name_ = this->input_argument_->name();
+	      *namep = name;
+	      return true;
+	    }
+	}

I think this code, which is nearly identical with that under "Case 4"
should be refactored into its own function (which does not need to be
in the class, either).

+  // an extra directory to look for the libraries passed by
+  // add_input_library.

Capitalize first word of the sentence. s/look/search/.

-cary