[CAGE] refactor type max/min detection out of config/auto/format.pm

[cotto]

There's currently code in config/auto/format.pm for some functions that determine which macros to use for PARROT_INTVAL_MAX, etc. The relevant functions, _set_intvalmaxmin() and _set_floatvalmaxmin() should be moved somewhere more appropriate. config/auto/sizes.pm looks like a better place, if yet another configure step is undesirable.

[jkeenan]

Replying to cotto:

There's currently code in config/auto/format.pm for some functions that determine which macros to use for PARROT_INTVAL_MAX, etc. The relevant functions, _set_intvalmaxmin() and _set_floatvalmaxmin() should be moved somewhere more appropriate.

Can you say a bit more about why config/auto/format.pm is not an appropriate location for these functions? (I don't understand the issues well, and I suspect others would benefit from clarification.)

Thank you very much.
kid51

[cotto]

Replying to jkeenan:

Replying to cotto:

There's currently code in config/auto/format.pm for some functions that determine which macros to use for PARROT_INTVAL_MAX, etc. The relevant functions, _set_intvalmaxmin() and _set_floatvalmaxmin() should be moved somewhere more appropriate.

Can you say a bit more about why config/auto/format.pm is not an appropriate location for these functions? (I don't understand the issues well, and I suspect others would benefit from clarification.) Thank you very much.
kid51

The purpose of the format step (config/auto/format.pm and associated C templates in config/auto/format/) is to deal with configuration options related to printf formatting. Figuring out maximum and minimum possible values for a given type don't have anything to do with that. It'd make a little more sense to put this code in the size step, but it should probably go in its own "limit" step.

Christoph

[jkeenan]

Replying to cotto:

Replying to jkeenan:

Replying to cotto:

The purpose of the format step (config/auto/format.pm and associated C templates in config/auto/format/) is to deal with configuration options related to printf formatting. Figuring out maximum and minimum possible values for a given type don't have anything to do with that. It'd make a little more sense to put this code in the size step, but it should probably go in its own "limit" step.

Thanks for responding. I will look into creating a separate config step after tomorrow's release.

kid51

[cotto]

jkeenan, I also noticed that config/auto/format/intval_maxmin_c.in seems to serve no purpose. If you run Configure.pl with --verbose-step=auto::format, you'll see that the file doesn't even compile. It wouldn't be very hard to make it compile again, but I wonder why it's even there, since its failure to compile isn't causing any problems.

[jkeenan]

Replying to cotto:

jkeenan, I also noticed that config/auto/format/intval_maxmin_c.in seems to serve no purpose. If you run Configure.pl with --verbose-step=auto::format, you'll see that the file doesn't even compile. It wouldn't be very hard to make it compile again, but I wonder why it's even there, since its failure to compile isn't causing any problems.

The file appears to have been added originally added in r31699 by julianalbo. Let's ask him about it.

kid51

[jkeenan]

Replying to cotto:

jkeenan, I also noticed that config/auto/format/intval_maxmin_c.in seems to serve no purpose. If you run Configure.pl with --verbose-step=auto::format, you'll see that the file doesn't even compile. It wouldn't be very hard to make it compile again, but I wonder why it's even there, since its failure to compile isn't causing any problems.

cotto, thanks for calling my attention to this. The more I look at this, the more I think that there are parts of two internal methods which are pointless:

sub _set_intvalmaxmin {
    my $conf = shift;
    my $ivmin;
    my $ivmax;
    ...
    $conf->data->set( intvalmin   => $ivmin );
    $conf->data->set( intvalmax   => $ivmax );

    $conf->cc_gen('config/auto/format/intval_maxmin_c.in');
    eval { $conf->cc_build(); };
    if ( $@ ) {
        $ivmin = '0';
        $ivmax = '0';
    }

}

...

sub _set_floatvalmaxmin {
    my $conf = shift;
    my $nvmin;
    my $nvmax;
    ...

    $conf->data->set( floatvalmin => $nvmin );
    $conf->data->set( floatvalmax => $nvmax );

    $conf->cc_gen('config/auto/format/floatval_maxmin_c.in');
    eval { $conf->cc_build(); };
    if ( $@ ) {
        $nvmin = '0';
        $nvmax = '0';
    }

}

Note that in each method, past the $conf->cc_gen() and $conf->cc_build() statements, nothing is ever stored in the Parrot::Configure object. But that object is the data store out of which the Makefile, lib/Parrot/Config/Generated.pm, etc. are built. Whether or not the $@ is populated in either method has nothing to do with the data available for build.

Do you agree?

kid51

[cotto]

I asked NotFound about the intval_maxmin_c.in stuff on #parrot, and he said that it's part of an unfinished effort to make sure that the min/max values result in something that compiles. I recommended that he file a tt about and add a comment so anyone else who looks at the code will know what's going on, but it looks like we have an explanation.

[jkeenan]

Replying to cotto:

There's currently code in config/auto/format.pm for some functions that determine which macros to use for PARROT_INTVAL_MAX, etc. The relevant functions, _set_intvalmaxmin() and _set_floatvalmaxmin() should be moved somewhere more appropriate. config/auto/sizes.pm looks like a better place, if yet another configure step is undesirable.

I have moved these internal methods to config/auto/sizes.pm. Some of the code is not covered by the test suite, so that remains to be done. But all current tests pass.

I moved intval_maxmin_c.in and floatval_maxmin_c.in to config/auto/sizes/, notwithstanding the problems described in this TT. However, since trunk is the place for more-or-less finished code and the code using those two probe files was admittedly incomplete, I've deleted that code from config/auto/sizes.pm. I'll open a separate TT to handle that.

Thank you very much.
kid51

[jkeenan]

Replying to jkeenan:

I moved intval_maxmin_c.in and floatval_maxmin_c.in to config/auto/sizes/, notwithstanding the problems described in this TT. However, since trunk is the place for more-or-less finished code and the code using those two probe files was admittedly incomplete, I've deleted that code from config/auto/sizes.pm. I'll open a separate TT to handle that.

See TT #582.

[jkeenan]

Replying to jkeenan:

I have moved these internal methods to config/auto/sizes.pm. Some of the code is not covered by the test suite, so that remains to be done. But all current tests pass.

Additional tests were added in r38377. Closing ticket.