> On Oct. 20, 2014, 10:56 a.m., Niklas Nielsen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1283 > > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1283> > > > > How about flatten it to os::libraries::expand() ? > > Kapil Arya wrote: > It's not clear what expand() would mean but expandName() is clearer. > expandLibraryName() would have been the best, though.
library is already a part of the namespace, so that would become redundant? I mostly suggested it because you won't have other conflicting expand(std::string name) in that namespace and it would be more concise. Anyway, it is not a biggy; feel free to drop the issue if you want to keep expandName > On Oct. 20, 2014, 10:56 a.m., Niklas Nielsen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1271 > > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1271> > > > > I looked through your patch set - it doesn't look like you use this > > function anywhere? > > > > It is a bit unclear to use 'ld library path' and 'path' > > interchangeably. How about sticking to 'path' or 'paths' (it is a path list > > right?) > > > > For example: > > > > // returns ':' separated list of library paths. > > os::libraries::paths() > > > > // Resets library variable to ':' separated library list. > > os::libraries::setPaths() > > > > // Appends library path to library list. > > os::libraries::addPath() > > > > Let's think about the path() overload for a little bit. I think for > > getting and setting, we should call out explicitly what we do ( > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Overloading). > > Kapil Arya wrote: > We use ldLibraryPath() to get the corresponding environment variable > string ("LD_LIBRARY_PATH"/"DYLD_LIBRARY_PATH"). > > I like the paths(), setPaths() and addPaths() function names and will > update the diff accordingly. Sure - but are you using it anywhere else? We expose two kinds of 'paths' - a ldLibraryPath (which is not a path but a env variable name) and 'path' which is a ':' separated directory list. You should be able to hide ldLibraryPath with your new abstraction, so that would make the api cleaner IMHO. Ben, what is your take? I am not strongly attached to this, so if you think they are distinct enough - feel free to drop. - Niklas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26903/#review57372 ----------------------------------------------------------- On Oct. 18, 2014, 12:04 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26903/ > ----------------------------------------------------------- > > (Updated Oct. 18, 2014, 12:04 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Also added a utility function to expand a library name to a filename > by prefixing "lib" and adding the default extension. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 63bda7a87e24e1696f3e7bb0206c551ce39dfe27 > > Diff: https://reviews.apache.org/r/26903/diff/ > > > Testing > ------- > > Ran make check. > > > Thanks, > > Kapil Arya > >
