Re: [PATCH] Support more C++ extensions for MSVC in the compile script.

[Peter Rosin]

Den 2010-08-13 19:08 skrev Ralf Wildenhues:
> Hi Peter,
> 
> * Peter Rosin wrote on Fri, Aug 13, 2010 at 03:37:59PM CEST:
>> This is one of the last bits of functionality from the (dwindling)
>> pr-msvc-support branch in Libtool (the remaining bit is triggering
>> the ar-lib script with a new macro and some things related to that).
> 
> Cool.
> 
>> MSVC only recognizes .c and .cpp as source files, if you say e.g.
>> foo.cc, it will treat it as an object file. Many projects do not use
>> .cpp for its C++ files, so this patch will make it work for a bunch
>> of more projects.
>>
>> I have not added .C to the list of C++ extensions as Windows typically
>> has a case insensitive file system, and then you don't know if the
>> user meant C or C++...
> 
> Good point.  Users will have to cope anyway.
> 
>> Ok for the msvc branch?
> 
> Ok with nits below addressed.  Thanks!

Pushed with changes, and merged into master.

>> +2010-08-13  Peter Rosin  <address@hidden>
>> +
>> +    Support more C++ extensions for MSVC in the compile script.
> 
> s/extensions/file &/  ?  IMHO that would make it a bit clearer.

Ok.

>> +    * lib/compile (func_cl_wrapper): MSVC only recognizes the .cpp
>> +    extension as C++, unless it's given a hint. So hint about
>> +    .cc, .cxx, c++ and C++. Also do path conversion on .c, .cpp
>> +    and .lib files.
> 
> See below for a question about this.
> 
>> +    * lib/compile3.test: Test the C++ hinting.
> 
>> --- a/lib/compile
>> +++ b/lib/compile
> 
>> @@ -129,6 +129,16 @@ func_cl_wrapper ()
>>        eat=1
>>        linker_opts="$linker_opts $2"
>>        ;;
> 
> Please add a case
>         -*)
>           set x "$@" "$1"
>           shift
>           ;;

I had that at one point, but have now added it back. I was never sure
about which form I liked better anyway...

> here and simplify the following matches accordingly.
> 
>> +    [!-]*.cc | [!-]*.cxx | [!-]*.[cC]++)
> 
> What about *.CC and *.CXX?
> 
>> +      func_file_conv "$1"
>> +      set x "$@" -Tp"$file"
>> +      shift
>> +      ;;
>> +    [!-]*.c | [!-]*.cpp | [!-]*.lib | [!-]*.LIB | [!-]*.Lib)
>> +      func_file_conv "$1"
> 
> What about *.CPP?

I made those changes too, thanks for looking!

> Why do you convert here?  Seems to me that since the files stand alone,
> MSYS conversion would kick in, no?  If this is about conversion on
> non-MSYS systems, please state so in the log entry.  Thanks.
> 
> Actually, there is an efficiency question in there: we know when exactly
> name conversion happens on MSYS, namely when the name is a positional
> parameter on its own (and in a couple of other cases which I have all
> but forgotten again).  In those cases, we could avoid doing manual
> conversion, thereby avoiding extra forks.
> 
> One way this could be implemented would be to pass an extra argument $2
> to func_file_conv that could be empty or "-lazy" or so, and in the
> function match against $file_conv$2 with cygwin-lazy same as cygwin but
> mingw-lazy a nop.
> 
> If you think this is worthwhile, then I would accept such a change in a
> later patch, given some numbers that show an actual improvement.  I can
> also write the patch if you do the measurement.  (Trying to stick to the
> "no optimization without measurement" principle.)

Given that it's at least five extra forks (``, uname, ``, cmd and sed)
per compile when a path translation is needed, I'd be surprised if it
didn't should show up on the radar. Even if bash is able to optimize away
one or two of those, it's still fork galore.

I'll write a patch and investigate...

Cheers,
Peter