Ticket #563 (closed cage: fixed)

Opened 6 years ago

Last modified 6 years ago

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

Reported by: cotto Owned by: jkeenan
Priority: minor Milestone: 1.2
Component: configure Version:
Severity: medium Keywords:
Cc: NotFound, particle Language:
Patch status: Platform:

Description

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.

Change History

in reply to: ↑ description ; follow-up: ↓ 2   Changed 6 years ago by 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

in reply to: ↑ 1 ; follow-up: ↓ 3   Changed 6 years ago by 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

in reply to: ↑ 2   Changed 6 years ago by jkeenan

  • owner set to jkeenan
  • status changed from new to assigned

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

  Changed 6 years ago by jkeenan

  • milestone set to 1.2

  Changed 6 years ago by jkeenan

  • component changed from none to configure

follow-ups: ↓ 7 ↓ 8   Changed 6 years ago by 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.

in reply to: ↑ 6   Changed 6 years ago by jkeenan

  • cc NotFound added

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

in reply to: ↑ 6   Changed 6 years ago by jkeenan

  • cc particle added

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

  Changed 6 years ago by 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.

in reply to: ↑ description ; follow-ups: ↓ 11 ↓ 12   Changed 6 years ago by 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

in reply to: ↑ 10   Changed 6 years ago by 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.

in reply to: ↑ 10   Changed 6 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

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.

Note: See TracTickets for help on using tickets.