Add native extensions for the Dart shell to mac and windows

Add native extensions for the Dart shell to the Windows platform and the Mac OS X platform.


BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=5235

[Ivan Posva, 2012-03-02 21:40:24]

This is not what we agreed to do. The biggest problem is that you need to
duplicate the API calls everywhere.

-Ivan

[Bill Hesse, 2012-03-05 09:20:39]

On 2012/03/02 21:40:24, Ivan Posva wrote:
> This is not what we agreed to do. The biggest problem is that you need to
> duplicate the API calls everywhere.
> 
> -Ivan

The problem is that I don't have a way to comment out the function bodies of the
implementation in the dart_api_impl.cc file.  I can't put /* into a macro, and
the tricks people have used for this don't work.

So even if I make the function bodies into static unused functions, which are
dropped by the linker, they still need to be compiled.

But then they need all of the VM implementation's header files, in order to
compile, because they refer to VM internals.
Many of the VM header files do not just provide class and function declarations,
and typedefs, but also inlined functions, which are not dropped by the linker,
and then have unfound references to symbols in the VM implementation.  So there
is no way to make the compiler accept the function bodies in dart_api_impl.cc
without linking in the VM.

We could have put the wrappers in dart_api.h, but we didn't want to make that
file ugly in any way.
If you have any ideas for how to make this work, please let me know - I tried to
make it work with the _impl.cc file
many ways last week.

[Søren Gjesse, 2012-03-05 12:50:11]

On 2012/03/05 09:20:39, Bill Hesse wrote:
> On 2012/03/02 21:40:24, Ivan Posva wrote:
> > This is not what we agreed to do. The biggest problem is that you need to
> > duplicate the API calls everywhere.
> > 
> > -Ivan
> 
> The problem is that I don't have a way to comment out the function bodies of
the
> implementation in the dart_api_impl.cc file.  I can't put /* into a macro, and
> the tricks people have used for this don't work.
> 
> So even if I make the function bodies into static unused functions, which are
> dropped by the linker, they still need to be compiled.
> 
> But then they need all of the VM implementation's header files, in order to
> compile, because they refer to VM internals.
> Many of the VM header files do not just provide class and function
declarations,
> and typedefs, but also inlined functions, which are not dropped by the linker,
> and then have unfound references to symbols in the VM implementation.  So
there
> is no way to make the compiler accept the function bodies in dart_api_impl.cc
> without linking in the VM.
> 
> We could have put the wrappers in dart_api.h, but we didn't want to make that
> file ugly in any way.
> If you have any ideas for how to make this work, please let me know - I tried
to
> make it work with the _impl.cc file
> many ways last week.

Could something like this work for having all in dart_api_impl.cc?

#ifdef _WIN32

#define API_FUNCTION(return_type, name, typed_arguments, arguments)            \
  extern "C" return_type name typed_arguments {                                \
  typedef return_type (* name ## PtrType) typed_arguments;                     \
  static name##PtrType name##Ptr = NULL;                                       \
  if (name##Ptr == NULL) {                                                     \
    name##Ptr = (name##PtrType) GetProcAddress(getDartModule(), #name);        \
  }                                                                            \
  assert(name##Ptr != NULL);                                                   \
  return (*name##Ptr) arguments;                                               \
}

#else

#define API_FUNCTION(return_type, name, typed_arguments, arguments)            \
  extern "C" return_type name typed_arguments {

#endif


// Dart_IsError
DART_EXPORT API_FUNCTION(bool, Dart_IsError, (Dart_Handle handle), (handle))
#ifndef _WIN32
  *** Non Windows body ***
  }
#endif

[Bill Hesse, 2012-03-05 14:02:30]

It could work to wrap all the implementation bodies in an ifdef wrapper, though
the ifdef should probably be
#ifndef WINDOWS_API_WRAPPER.

If that is the preferred solution, I can do that (somewhat) easily

On 2012/03/05 12:50:11, Søren Gjesse wrote:
> On 2012/03/05 09:20:39, Bill Hesse wrote:
> > On 2012/03/02 21:40:24, Ivan Posva wrote:
> > > This is not what we agreed to do. The biggest problem is that you need to
> > > duplicate the API calls everywhere.
> > > 
> > > -Ivan
> > 
> > The problem is that I don't have a way to comment out the function bodies of
> the
> > implementation in the dart_api_impl.cc file.  I can't put /* into a macro,
and
> > the tricks people have used for this don't work.
> > 
> > So even if I make the function bodies into static unused functions, which
are
> > dropped by the linker, they still need to be compiled.
> > 
> > But then they need all of the VM implementation's header files, in order to
> > compile, because they refer to VM internals.
> > Many of the VM header files do not just provide class and function
> declarations,
> > and typedefs, but also inlined functions, which are not dropped by the
linker,
> > and then have unfound references to symbols in the VM implementation.  So
> there
> > is no way to make the compiler accept the function bodies in
dart_api_impl.cc
> > without linking in the VM.
> > 
> > We could have put the wrappers in dart_api.h, but we didn't want to make
that
> > file ugly in any way.
> > If you have any ideas for how to make this work, please let me know - I
tried
> to
> > make it work with the _impl.cc file
> > many ways last week.
> 
> Could something like this work for having all in dart_api_impl.cc?
> 
> #ifdef _WIN32
> 
> #define API_FUNCTION(return_type, name, typed_arguments, arguments)           
\
>   extern "C" return_type name typed_arguments {                               
\
>   typedef return_type (* name ## PtrType) typed_arguments;                    
\
>   static name##PtrType name##Ptr = NULL;                                      
\
>   if (name##Ptr == NULL) {                                                    
\
>     name##Ptr = (name##PtrType) GetProcAddress(getDartModule(), #name);       
\
>   }                                                                           
\
>   assert(name##Ptr != NULL);                                                  
\
>   return (*name##Ptr) arguments;                                              
\
> }
> 
> #else
> 
> #define API_FUNCTION(return_type, name, typed_arguments, arguments)           
\
>   extern "C" return_type name typed_arguments {
> 
> #endif
> 
> 
> // Dart_IsError
> DART_EXPORT API_FUNCTION(bool, Dart_IsError, (Dart_Handle handle), (handle))
> #ifndef _WIN32
>   *** Non Windows body ***
>   }
> #endif

[Ivan Posva, 2012-03-06 00:28:31]

Personally I do not have much of an opinion how you ensure that the stub library
and the actual implementation are in sync, but it should be at least a Windows
build failure if the files are out of sync. This requirement should leave a
couple avenues open for you to implement this properly. What I really want to
avoid isa situation where we push out a build without knowing that the stub
library is out-of-date.
- You can do this by either generating the stub library code automatically in a
fashion similar to what we discussed or by running some script during build time
on Windows.
- You can write a test that verifies that all of the exported symbols (functions
in the header files) are present in the stub library which can run as a test on
Windows.
- Some other way...

Please outline your plan before we continue with this change.

Thanks,
-Ivan

P.S. I understand that my original suggestion of using a macro similar to
DEFINE_NATIVE_ENTRY would need major restructuring and cleanup in the
dart_api_impl.cc file which could be outside the scope of this change.


On 2012/03/05 14:02:30, Bill Hesse wrote:
> It could work to wrap all the implementation bodies in an ifdef wrapper,
though
> the ifdef should probably be
> #ifndef WINDOWS_API_WRAPPER.
> 
> If that is the preferred solution, I can do that (somewhat) easily
> 
> On 2012/03/05 12:50:11, Søren Gjesse wrote:
> > On 2012/03/05 09:20:39, Bill Hesse wrote:
> > > On 2012/03/02 21:40:24, Ivan Posva wrote:
> > > > This is not what we agreed to do. The biggest problem is that you need
to
> > > > duplicate the API calls everywhere.
> > > > 
> > > > -Ivan
> > > 
> > > The problem is that I don't have a way to comment out the function bodies
of
> > the
> > > implementation in the dart_api_impl.cc file.  I can't put /* into a macro,
> and
> > > the tricks people have used for this don't work.
> > > 
> > > So even if I make the function bodies into static unused functions, which
> are
> > > dropped by the linker, they still need to be compiled.
> > > 
> > > But then they need all of the VM implementation's header files, in order
to
> > > compile, because they refer to VM internals.
> > > Many of the VM header files do not just provide class and function
> > declarations,
> > > and typedefs, but also inlined functions, which are not dropped by the
> linker,
> > > and then have unfound references to symbols in the VM implementation.  So
> > there
> > > is no way to make the compiler accept the function bodies in
> dart_api_impl.cc
> > > without linking in the VM.
> > > 
> > > We could have put the wrappers in dart_api.h, but we didn't want to make
> that
> > > file ugly in any way.
> > > If you have any ideas for how to make this work, please let me know - I
> tried
> > to
> > > make it work with the _impl.cc file
> > > many ways last week.
> > 
> > Could something like this work for having all in dart_api_impl.cc?
> > 
> > #ifdef _WIN32
> > 
> > #define API_FUNCTION(return_type, name, typed_arguments, arguments)         
 
> \
> >   extern "C" return_type name typed_arguments {                             
 
> \
> >   typedef return_type (* name ## PtrType) typed_arguments;                  
 
> \
> >   static name##PtrType name##Ptr = NULL;                                    
 
> \
> >   if (name##Ptr == NULL) {                                                  
 
> \
> >     name##Ptr = (name##PtrType) GetProcAddress(getDartModule(), #name);     
 
> \
> >   }                                                                         
 
> \
> >   assert(name##Ptr != NULL);                                                
 
> \
> >   return (*name##Ptr) arguments;                                            
 
> \
> > }
> > 
> > #else
> > 
> > #define API_FUNCTION(return_type, name, typed_arguments, arguments)         
 
> \
> >   extern "C" return_type name typed_arguments {
> > 
> > #endif
> > 
> > 
> > // Dart_IsError
> > DART_EXPORT API_FUNCTION(bool, Dart_IsError, (Dart_Handle handle), (handle))
> > #ifndef _WIN32
> >   *** Non Windows body ***
> >   }
> > #endif

[Bill Hesse, 2012-03-06 15:04:06]

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win.cc
File runtime/bin/extensions_win.cc (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:16: fprintf(stderr, "Reached 1\n");
Remove this fprintf.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:22: printf("%s\n", library_path);
Remove this printf.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/test_extension.cc
File runtime/bin/test_extension.cc (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/test_extension....
runtime/bin/test_extension.cc:13: return parent_library;
Reformat to longer lines.

[Mads Ager (google), 2012-03-06 15:32:50]

For context: Bill figured out today that we do not have to generate the link
library ourselves on Windows. It turns out that if you just give the linker a
flag identifying one symbol to be exported, it will generate both the .exe and
the .lib for you. Therefore, we do not need any type of macro magic to make
extensions work on Windows.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/bin.gypi#newcod...
runtime/bin/bin.gypi:345: 'msvs_settings': {
Please add a comment that this linker settings forces the generation of a link
library.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win.cc
File runtime/bin/extensions_win.cc (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:21: snprintf(library_path, buffer_length,
"%s.dll", extension_name);
On the other platforms the dll has to be called libextension_name.extension. We
should make it consistent and either require the 'lib' prefix here as well or
remove the requirement from linux and mac.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:31: (InitPtrType)GetProcAddress(extension_dll,
library_init_function);
Please use reinterpret_cast

[Søren Gjesse, 2012-03-07 06:58:31]

lgtm

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/bin.gypi#newcod...
runtime/bin/bin.gypi:345: 'msvs_settings': {
Should we perhaps add the same for the dart_no_snapshot target below?

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win.cc
File runtime/bin/extensions_win.cc (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:35: return (*initPtr)(parent_library);
reinterpret_cast here as well.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/test_extension.cc
File runtime/bin/test_extension.cc (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/test_extension....
runtime/bin/test_extension.cc:23: DART_EXTERN_C void IfNull(Dart_NativeArguments
arguments) {
Should we add an additional macro DART_NATIVE (or something like that) in place
of DART_EXTERN_C for the extension writer to use?

[Ivan Posva, 2012-03-07 07:22:50]

Thanks for figuring out a way that is not as horrible as building our own stub
library.

-Ivan

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/bin.gypi#newcod...
runtime/bin/bin.gypi:345: 'msvs_settings': {
On 2012/03/07 06:58:31, Søren Gjesse wrote:
> Should we perhaps add the same for the dart_no_snapshot target below?

The dart_no_snapshot target is not shipped as part of the SDK and is generally
only used during development of the VM. In particular during development of
first stages of loading sources. Just avoid the complication of creating link
library.

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/test_extension.cc
File runtime/bin/test_extension.cc (right):

http://codereview.chromium.org/9465004/diff/22001/runtime/bin/test_extension....
runtime/bin/test_extension.cc:33: DART_EXTERN_C Dart_NativeFunction
ResolveName(Dart_Handle name, int argc) {
Can you please explain why these need to be 'extern "C"'? All the registered
callback can be mangled all you want, as they are never looked up by name.

[Bill Hesse, 2012-03-07 14:11:30]

OK, I think all comments have been addressed adequately.

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/test_ex...
File runtime/bin/test_extension.cc (right):

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/test_ex...
runtime/bin/test_extension.cc:8: DART_EXTERN_C Dart_NativeFunction
ResolveName(Dart_Handle name, int argc);
Remove dart_extern_c

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/test_ex...
runtime/bin/test_extension.cc:18: 
Also here

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/test_ex...
runtime/bin/test_extension.cc:28: DART_EXTERN_C Dart_NativeFunction
ResolveName(Dart_Handle name, int argc) {
And here.

[Mads Ager (google), 2012-03-07 14:31:48]

This is getting very close. A bunch of minor comments that we should address and
then we are ready to land it.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc
File runtime/bin/extensions.cc (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:5: #include "bin/extensions.h"
We usually have a blank line after this include before the system includes.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:15:
assert(DartUtils::IsDartExtensionSchemeURL(extension_url));
Please use ASSERT.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:27: char* init_function_name = MakeString("%s_Init",
library_name);
I think inlining this code here would be just as clear. In particular because
MakeString is very specific about its arguments to the point where the
abstraction doesn't make that much sense?

If we want an abstraction for these, maybe we should provide a StringConcat
function instead that takes an array of char*s and concatenates them?

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:39: /* Returns a newly allocated string by printing
name using format.
Please use '//' style comments.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h
File runtime/bin/extensions.h (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h#ne...
runtime/bin/extensions.h:17: static char* MakeString(const char* format, const
char* name);
This one should not be exposed in the Extensions class. It is very specific
about the format. I would either make it a static helper method in the
implementation file or just inline the code.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h#ne...
runtime/bin/extensions.h:20: static void* LoadExtensionLibrary(const char*
library_name);
Could these two be private?

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_maco...
File runtime/bin/extensions_macos.cc (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_maco...
runtime/bin/extensions_macos.cc:9: char* library_path = MakeString("./%s.so",
library_name);
lib%s.so?

Either that or remove the lib prefix on linux.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_win.cc
File runtime/bin/extensions_win.cc (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:8: char* library_path = MakeString("%s.dll",
library_name);
lib%s.dll

Either that or remove the lib prefix on linux.

[Ivan Posva, 2012-03-07 19:06:23]

Also please fix the description of the change now that it is more that just
implementing native extensions on Windows.

-Ivan

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/extensi...
File runtime/bin/extensions.h (right):

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/extensi...
runtime/bin/extensions.h:16: Dart_Handle library);
library -> parent_library
Without any documentation it is entirely unclear what library you expect here.
The .cc file actually uses parent_library.

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/extensi...
File runtime/bin/extensions_macos.cc (right):

https://chromiumcodereview.appspot.com/9465004/diff/25001/runtime/bin/extensi...
runtime/bin/extensions_macos.cc:9: char* library_path = MakeString("./%s.so",
library_name);
On 2012/03/07 14:31:48, Mads Ager wrote:
> lib%s.so?
> 
> Either that or remove the lib prefix on linux.

Please do not use .so extensions on Mac OS X. It is definitely not the right
extension. The equivalent of .so on Mac OS X is .dylib.

[Bill Hesse, 2012-03-08 12:04:37]

The standard names that gyp produces for a project foo that produces a shared
library (or a bundle on Mac OS) are:
libfoo.so on linux
foo.so on mac
foo.dll on windows.

If we were producing a dynamic library on mac, it would be called foo.dylib, but
we are not.

All of the documentation, including Apple's, says that the extension on a bundle
is chosen by the app developer, and Chromium and python are both choosing .so.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc
File runtime/bin/extensions.cc (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:5: #include "bin/extensions.h"
On 2012/03/07 14:31:48, Mads Ager wrote:
> We usually have a blank line after this include before the system includes.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:15:
assert(DartUtils::IsDartExtensionSchemeURL(extension_url));
On 2012/03/07 14:31:48, Mads Ager wrote:
> Please use ASSERT.

Done.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:27: char* init_function_name = MakeString("%s_Init",
library_name);
Did you see all the uses of MakeString in the platform-specific files?

On 2012/03/07 14:31:48, Mads Ager wrote:
> I think inlining this code here would be just as clear. In particular because
> MakeString is very specific about its arguments to the point where the
> abstraction doesn't make that much sense?
> 
> If we want an abstraction for these, maybe we should provide a StringConcat
> function instead that takes an array of char*s and concatenates them?

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.cc#n...
runtime/bin/extensions.cc:39: /* Returns a newly allocated string by printing
name using format.
On 2012/03/07 14:31:48, Mads Ager wrote:
> Please use '//' style comments.

Done.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h
File runtime/bin/extensions.h (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h#ne...
runtime/bin/extensions.h:16: Dart_Handle library);
On 2012/03/07 19:06:23, Ivan Posva wrote:
> library -> parent_library
> Without any documentation it is entirely unclear what library you expect here.
> The .cc file actually uses parent_library.

Done.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h#ne...
runtime/bin/extensions.h:17: static char* MakeString(const char* format, const
char* name);
On 2012/03/07 14:31:48, Mads Ager wrote:
> This one should not be exposed in the Extensions class. It is very specific
> about the format. I would either make it a static helper method in the
> implementation file or just inline the code.

It is used in both the platform-specific and platform-independent files, so it
cannot be a static helper method (unless it is in the header file).  It really
cleans up the implementation to have this - otherwise, we are tempted to share
the same buffer for both things, and need to pass it into the platform-specific
functions.

It should be private, though.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions.h#ne...
runtime/bin/extensions.h:20: static void* LoadExtensionLibrary(const char*
library_name);
On 2012/03/07 14:31:48, Mads Ager wrote:
> Could these two be private?

Yes.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_maco...
File runtime/bin/extensions_macos.cc (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_maco...
runtime/bin/extensions_macos.cc:9: char* library_path = MakeString("./%s.so",
library_name);
libfoo.so on linux and foo.dll on windows is the naming system that is
universally used by almost everyone doing cross-platform libraries and
extensions.  On mac, Python and gyp do foo.so, and people making shared
libraries (not bundles) do libfoo.dylib (but we aren't making dylibs).
Can we get some consensus on the naming?  I really feel we should go with what
gyp produces.



On 2012/03/07 14:31:48, Mads Ager wrote:
> lib%s.so?
> 
> Either that or remove the lib prefix on linux.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_maco...
runtime/bin/extensions_macos.cc:9: char* library_path = MakeString("./%s.so",
library_name);
On 2012/03/07 19:06:23, Ivan Posva wrote:
> On 2012/03/07 14:31:48, Mads Ager wrote:
> > lib%s.so?
> > 
> > Either that or remove the lib prefix on linux.
> 
> Please do not use .so extensions on Mac OS X. It is definitely not the right
> extension. The equivalent of .so on Mac OS X is .dylib.

This is not a .dylib.  This is a bundle, not a shared library.  These are never
called .dylib.

Gyp and Python call their bundles .so.  The Apple documentation says they can
have any extension that you choose, like .bundle or one that your application
chooses.
Our make build chooses the extension .dylib for dynamic libraries and .so for
bundles.

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_win.cc
File runtime/bin/extensions_win.cc (right):

http://codereview.chromium.org/9465004/diff/25001/runtime/bin/extensions_win....
runtime/bin/extensions_win.cc:8: char* library_path = MakeString("%s.dll",
library_name);

I haven't seen any places where a DLL is called  lib<name>.dll.  Everybody calls
a .so  on linux libfoo.so.
This is the standard naming convention used in Chromium.

[Mads Ager (google), 2012-03-08 12:31:29]

I still don't like MakeString.

I would be fine with aligning this with what 'gyp' generates for the library
types for now. We should look into what we can make gyp do for us. I think the
'lib' prefix on Linux might confuse people.

http://codereview.chromium.org/9465004/diff/31001/runtime/bin/extensions.h
File runtime/bin/extensions.h (right):

http://codereview.chromium.org/9465004/diff/31001/runtime/bin/extensions.h#ne...
runtime/bin/extensions.h:19: static char* MakeString(const char* format, const
char* name);
I did see the usages and I really don't like the generic name and the fact that
it takes a format that has be be very specific. String functions that take a
format usually uses varargs as well. I would rather have 

char* Concat(const char** strings);

which is general and should be easy to use in all the places where you use the
current MakeString. 

// Concatenates a NULL terminated array of strings.
char* Concat(const char** strings) {
  int size = 1;  // null termination.
  for (int i = 0; strings[i] != NULL; i++) {
    size += strlen(strings[i]);
  }
  char* result = reinterpret_cast<char*>(malloc(size));
  int index = 0;
  for (int i = 0; strings[i] != NULL; i++) {
    index += snprintf(result + index, size - index, "%s", strings[i]);
  }
  ASSERT(index == size - 1);
  ASSERT(result[size - 1] == '\0');
  return result;
}

Usages would be something like:

void MyMethod(const char* libname) {
  const char* strings[4] = { "lib", libname, ".so", NULL};
  UseTheConstructedName(Extensions::Concat(strings));
}

[Bill Hesse, 2012-03-08 14:09:29]

MakeString removed and replaced with Concatenate.

[Mads Ager (google), 2012-03-08 14:12:16]

OK, LGTM

https://chromiumcodereview.appspot.com/9465004/diff/36001/runtime/bin/extensi...
File runtime/bin/extensions.h (right):

https://chromiumcodereview.appspot.com/9465004/diff/36001/runtime/bin/extensi...
runtime/bin/extensions.h:19: // The returned string must be freed.
Maybe add a comment about the requirements on the input as well?

[Ivan Posva, 2012-03-08 17:03:04]

Bill,

Go ahead with this change, but please file a bug against yourself to resolve the
outstanding inconsitencies.

-Ivan

https://chromiumcodereview.appspot.com/9465004/diff/36001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

https://chromiumcodereview.appspot.com/9465004/diff/36001/runtime/bin/bin.gyp...
runtime/bin/bin.gypi:449: 'type': 'shared_library',
Why are windows and linux not using loadable_module?

[Bill Hesse, 2012-03-09 13:24:52]

All three builds are using shared_library now, and the
from-scratch simple xcode build of a dylib, and the gyp build of a dylib,
both produce libfoo.dylib, which is what we are using for a name on MacOS.



On 2012/03/08 17:03:04, Ivan Posva wrote:
> Bill,
> 
> Go ahead with this change, but please file a bug against yourself to resolve
the
> outstanding inconsitencies.
> 
> -Ivan
> 
> https://chromiumcodereview.appspot.com/9465004/diff/36001/runtime/bin/bin.gypi
> File runtime/bin/bin.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/9465004/diff/36001/runtime/bin/bin.gyp...
> runtime/bin/bin.gypi:449: 'type': 'shared_library',
> Why are windows and linux not using loadable_module?