Re: Reduce the number of special cases to build contrib modules on windows

On Wed, 28 Jul 2021 at 01:44, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> I don't know anything about the MSVC build process, but I figured I
> could do a general Perl code review.

Thanks for looking at this. Perl review is very useful as it's
certainly not my native tongue, as you might have noticed.

> > --- a/src/tools/msvc/Mkvcbuild.pm
> > +++ b/src/tools/msvc/Mkvcbuild.pm
> […]
> > +     # Process custom compiler flags
> > +     if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg || $mf =~ /^override\s*CPPFLAGS\s*(?:[\+\:])?=\s*(.*)$/mg)
>                                                                                   ^^^^^^^^^^^
> This is a very convoluted way of writing [+:]?

I've replaced the (?:[\+\:])? with [+:]? It's a bit of a blind
adjustment. I see that the resulting vcxproj files have not changed as
a result of that.

> > --- a/src/tools/msvc/Project.pm
> > +++ b/src/tools/msvc/Project.pm
> > @@ -58,7 +58,7 @@ sub AddFiles
> >
> >       while (my $f = shift)
> >       {
> > -             $self->{files}->{ $dir . "/" . $f } = 1;
> > +             AddFile($self, $dir . "/" . $f, 1);
>
> AddFile is a method, so should be called as $self->AddFile(…).

Adjusted thanks.

> > --- a/src/tools/msvc/Mkvcbuild.pm
> > +++ b/src/tools/msvc/Mkvcbuild.pm
> > @@ -36,16 +36,12 @@ my @unlink_on_exit;
> […]
> > +                     elsif ($flag =~ /^-I(.*)$/)
> > +                     {
> > +                             foreach my $proj (@projects)
> > +                             {
> > +                                     if ($1 eq '$(libpq_srcdir)')
> > +                                     {
> > +                                             $proj->AddIncludeDir('src\interfaces\libpq');
> > +                                             $proj->AddReference($libpq);
> > +                                     }
> > +                             }
> > +                     }
>
> It would be better to do the if check outside the for loop.

Agreed.

> > --- a/src/tools/msvc/Project.pm
> > +++ b/src/tools/msvc/Project.pm
> > @@ -51,6 +51,16 @@ sub AddFile
> >       return;
> >  }
> >
> > +sub AddFileAndAdditionalFiles
> > +{
> > +     my ($self, $filename) = @_;
> > +     if (FindAndAddAdditionalFiles($self, $filename) != 1)
>
> Again, FindAndAddAdditionalFiles is a method and should be called as
> $self->FindAndAddAdditionalFiles($filename).
>
> > +     {
> > +             $self->{files}->{$filename} = 1;
> > +     }
> > +     return;
> > +}

Adjusted.

I'll send updated patches once I look at the other reviews.

David