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
