Re: Review of VS 2010 support patches

On 09/28/2011 03:53 PM, Brar Piening wrote:
> Brar Piening wrote:
>>
>> The attached patch includes documentation changes and excludes my
>> versions of pgbison.pl and pgflex.pl which have been replaced by
>> Andrews' versions that are already commited.
>
> Building current head today I noticed that the patch doesn't apply
> cleanly anymore.
>
> Attached is a new version.
>
>

This patch looks OK in principle.

Some minor nitpicks:

Do we really need to create all those VSnnnnProject.pm and
VSnnnnSolution.pm files? They are all always included anyway. Why not
just stash all the packages in Solution.pm and Project.pm? Also, instead
of doing this in Mkvcbuild.pm:

my $vsVersion = VSObjectFactory::DetermineVisualStudioVersion();
$solution = VSObjectFactory::CreateSolution($vsVersion, $config);

why not just add "use VSObjectFactory;" at the top of the file and
import these into the current namespace, just as we do for pretty much
everything else?

There are some stylistic things that aren't the way I usually do things
(use of named instead of anonymous file handles, use of heredocs instead
of qq{} style quotes) and that I would prefer done differently, but
those are more matters of taste than substance. I also generally dislike
composing XML by non-formal means, as it can be quite error prone and
often leads to errors in unforeseen corner cases. But in this case we
certainly don't want to impose an extra requirement on some perl XML
module, and it would make this code terribly verbose, so we just have to
hope we get it right :-)

I don't have a VS2010 machine available to test it on unfortunately.
I'll see what I can do about arranging one, at least temporarily.
Meanwhile I'll test it on my VS2005 and VS2008 machines to make sure it
doesn't break anything.

cheers

andrew