Use parser for input parameters of type long

[NeilZaim]

Two integer input parameters (<species_name>.npart for a gaussian_beam injection_style and <laser_name>.min_particles_per_mode) are currently not read by the math parser because they are of type long.

So that they could be read (and more generally that long input parameters can be read), I've made the following modifications:

• The queryWithParser functions that read integers are now templated so that they can be reused for long.
• The safeCastToInt function that is used by the above functions is also templated, with int as the default template parameter.
• All of these functions have been moved to the bottom of WarpXUtil.H because they need functions declared in that file. That probably makes the PR harder to read, sorry about that.

@dpgrote Do you think that this is a good solution to parse long input parameters?

[ax3l]

I would prefer if we maybe use long/amrex::Long by default here.

Otherwise we start carrying more and more basic functionality into headers - which included in every translation unit that uses it will increase compile time.

One way to solve this if you need two types is also: we can use this template construct inside the WarpXUtil.cpp file and add a safeCastToInt and safeCastToLong function declaration here in the header.

[dpgrote]

This looks Ok. Though, note that there are already duplicated routines for float and double. Perhaps follow the same model and create duplicate routines for int and long instead of using templates?

[NeilZaim]

Thanks for your comments. I agree with you that it's better to avoid the significant increase in compile time associated to these templates.

One thing we could do is to use explicit template instantiation: declare the templates in WarpXUtil.H, define them in WarpXUtil.cpp and instantiate them with the needed type (so here int and long) in WarpXUtil.cpp. That way, these functions are only compiled in that single translation unit and we still avoid code duplication. However, I don't think I've ever seen this construct (explicit template instantiation) in WarpX so maybe there's something wrong with it or it's considered bad practice?

Otherwise, we can just duplicate the functions for int and long as @dpgrote suggested.

One way to solve this if you need two types is also: we can use this template construct inside the WarpXUtil.cpp file and add a safeCastToInt and safeCastToLong function declaration here in the header.

Looks like there may be a third option, but I don't think I understand it 😄. If we declare the functions with two different names, then we can no longer define them using templates, can we?

[ax3l]

Thanks, sounds great!

Thanks for your comments. I agree with you that it's better to avoid the significant increase in compile time associated to these templates.

Yes just to clarify, we want to avoid to establish a pattern that is compile-time intensive. Here the small change would not change the world (yet).

One thing we could do is to use explicit template instantiation: declare the templates in WarpXUtil.H, define them in WarpXUtil.cpp and instantiate them with the needed type (so here int and long) in WarpXUtil.cpp. That way, these functions are only compiled in that single translation unit and we still avoid code duplication. However, I don't think I've ever seen this construct (explicit template instantiation) in WarpX so maybe there's something wrong with it or it's considered bad practice?

As long as the body is implemented in the .H file, all that a specialization (like this) in a .cpp file is ensuring is that the symbols for that specialization are truly created and part of your binary. So it won't reduce compile time for any other file that includes the header and calls the function.

(I also experimented with extern declarations in the past, but compilers are allowed to ignore them to inline functions they find at compile time. Compilers do ignore that for all but -O0, as I found.)

Looks like there may be a third option, but I don't think I understand it smile. If we declare the functions with two different names, then we can no longer define them using templates, can we?

Yes, let me elaborate. You can write two "stub" declarations in the header file safeCastToInt and safeCastToLong. Then in the .cpp file, you write a helper function with a template that is the same as your current .H implementation of safeCastToInt<T> - and then you call this function in the .cpp file in the bodies of safeCastToInt and safeCastToLong.

// .H
int safeCastToInt(...); // ... is the args as before
int safeCastToLong(...);

// .cpp
namespace detail {
   template< typename T >
   AMREX_FORCE_INLINE
   T safeCastTo( ... ) {
      // ... body as before
   }
} // namespace detail

int safeCastToInt( ... ) { return detail::safeCastTo< amrex::Int >( ... ); }
int safeCastToLong( ... ) { return detail::safeCastToLong< amrex::Long >( ... ); }

That way, you have the best of those worlds: saved compile time and general code reuse.

[NeilZaim]

@ax3l Ok thanks, I understand your solution, that would work as well.

As long as the body is implemented in the .H file, all that a specialization (like this) in a .cpp file is ensuring is that the symbols for that specialization are truly created and part of your binary. So it won't reduce compile time for any other file that includes the header and calls the function.

No, I'm talking about an explicit template instantiation where I put the body of the templated functions in the .cpp file, so that the other files cannot compile these functions. Basically, with your notations, that would look like:

// .H
// Declaration
template <typename int_type = int>
int_type safeCastToInt (...);

// .cpp
// Definition
template <typename int_type = int>
int_type safeCastToInt (...)
{
// body as before
}

// Explicit template instantiation
template int safeCastToInt (...);
template long safeCastToInt (...);

I was wondering if there was an issue in doing that? (I have checked and at least the code compiles).

In the end, both solutions are very similar. There may be fewer intermediate functions with an explicit template instantiation, but I'm definitely fine with both.

[ax3l]

You are right, for fully specialized templates this totally works. You just cannot inline them, but that's not an issue here at all.

[NeilZaim]

Sounds good, I will do that. Putting [WIP] until that's done.

[NeilZaim]

@ax3l @dpgrote Sorry I had forgotten about this PR. It should now be ready for review.

Looks like in the meantime the getWithParser and queryWithParser functions have already been templated (in #3065) so in this PR I now only need to add a safeCastToLong function (implemented as @ax3l suggested by adding a templated function in a namespace in the .cpp file, to avoid code duplication). We can potentially do something similar for the getWithParser/queryWithParser functions if we want to save compile time.

[dpgrote]

Typo "aa long"

[NeilZaim]

Thanks for catching this, this is fixed now.

[dpgrote]

This looks good, thanks! I have the one minor comment.