Allow std::string "name" arguments for ParmParse methods

[charlesgwaldman]

Summary

The ParmParse constructor accepts argument prefix == std::string. But once the object is created, methods on it require char * objects for the name argument, leading to code like:

std::string prefix = "prefix";
std::string name = "name";
ParmParse pp(prefix);
pp.get(name.c_str(), input);

The proposed patch adds wrappers to allow arguments to ParmParse methods get, query, etc, to allow the name argument to be a std::string, so the c_str() is not needed and the interface is consistent. It also fixes a few mismatches between comments and code.

Additional background

Wrappers are provided for all methods except one. It was not possible to add a std::string interface for the two-name version of get:

    void get (const char* new_name, const char* old_name, T& ref)

because with string values for the first two arguments and an int value for the third, this creates an ambiguity with the standard get:

     void get (const std::string&  name,
              std::string& ref,
              int          ival = FIRST)

For this reason, a wrapper is not provided for the two-name version of get.
Currently the only thing that disambiguates this template from the standard get is that in the two-name version both names are char* and in the template the second argument ref is std::string, allowing the first argument to be string breaks this. This is an unforunate feature of the API design.
Furthermore I believe this function is somewhat obscure and it should possibly be deprecated or renamed to get_with_fallback or get2 or similar.

Checklist

The proposed changes:

• fix a bug or incorrect behavior in AMReX
• add new capabilities to AMReX
• changes answers in the test suite to more than roundoff level
• are likely to significantly affect the results of downstream AMReX users
• include documentation in the code and/or rst files, if appropriate

[WeiqunZhang]

If we want to support std::string directly, we probably could use std::string_view. Something like

diff --git a/Src/Base/AMReX_ParmParse.H b/Src/Base/AMReX_ParmParse.H
index 6fa8e1ad10..ab1b41acca 100644
--- a/Src/Base/AMReX_ParmParse.H
+++ b/Src/Base/AMReX_ParmParse.H
@@ -432,7 +432,7 @@ public:
                   int&        ref,
                   int         ival = FIRST) const;
     //! Same as querykth() but searches for the last occurrence of name.
-    int query (const char* name,
+    int query (std::string_view name,
                int&        ref,
                int         ival = FIRST) const;
     //! Add a key 'name'with value 'ref' to the end of the PP table.
diff --git a/Src/Base/AMReX_ParmParse.cpp b/Src/Base/AMReX_ParmParse.cpp
index 8793f869d8..ae421153ac 100644
--- a/Src/Base/AMReX_ParmParse.cpp
+++ b/Src/Base/AMReX_ParmParse.cpp
@@ -1424,7 +1424,7 @@ ParmParse::querykth (const char* name, int k, int& ref, int ival) const
 }
 
 int
-ParmParse::query (const char* name, int& ref, int ival) const
+ParmParse::query (std::string_view name, int& ref, int ival) const
 {
     return squeryval(*m_table,m_parser_prefix, prefixedName(name),ref,ival, LAST);
 }

[charlesgwaldman]

Why is string_view better than string? My patch uses std::string, I can change it to string_view but I'd like to understand the rationale.

[WeiqunZhang]

You might have misunderstood my comments. The string_vew version is to replace rather supplement the const char* version.

For example,

int f(std::string_view); // only ONE version

is arguably better than

int f(const char*);
// AND
int f(std::string const&);

[WeiqunZhang]

As for void get (const char* new_name, const char* old_name, T& ref) and void query (const char* new_name, const char* old_name, T& ref), just keep them as they are will avoid ambiguity. They are being used. So don't rename them.

[charlesgwaldman]

Thank you for the clarification.

[charlesgwaldman]

I modified the code to use string_view as suggested. Reopening this pull request.

[charlesgwaldman]

@WeiqunZhang

The approach with "string_view" caused a lot of tests to fail.

One issue is that existing code which already calls c_str (e.g.

AMReX_HypreIJIface.cpp:27
if (pp.contains(key.c_str())) 

generates a "redundant call to c_str" warning

error: redundant call to 'c_str' [readability-redundant-string-cstr,-warnings-as-errors]

Another issue is that the pyamrex build is failing:

/home/runner/work/amrex/amrex/pyamrex/src/Base/ParmParse.cpp: In function ‘void init_ParmParse(pybind11::module&)’:
/home/runner/work/amrex/amrex/pyamrex/src/Base/ParmParse.cpp:38:63: error: no match for call to ‘(const pybind11::detail::overload_cast_impl<const char*, const bool>) (<unresolved overloaded function type>)’
   38 |         .def("add", py::overload_cast<char const*, bool const>(&ParmParse::add))
      |                     

I don't think the string_view method can work without breaking existing code. I think my original approach with wrapper functions in AMREX_ParmParse.H to handle std::string name arguments would succeed without causing the above problems.

If you would be willing to accept the patch with wrapper functions, I will go back to that version. Otherwise I will respectfully close this MR and live with the c_str requirement.

Please advise,
Thank you.

[WeiqunZhang]

The warnings are easy to fix. As for pyamrex, once this is merge, we can fix it.

In CI tests, we intentionally treat warnings as errors so that we can fix the warnings. It will not break the user's code unless they also treat warnings as errors.

[charlesgwaldman]

Thank you. I understand that the AMReX CI uses -Werror to treat warnings as errors (we do the same with the MFIX-EXA CI).

If you are not worried about user code producing warnings, I will continue with the string_view approach.

[WeiqunZhang]

Sounds good. Those warnings are not even compiler warnings. They are from clang-tidy, which is even more strict on styles.

[WeiqunZhang]

The c_str() here is also redundant.

[charlesgwaldman]

Yup, I missed a few redundant c_str()'s the first time, in EB/CNS/Source/CNS.cpp and EB_CNS/Source/CNS.cpp. I fixed all of those on my branch. Everything passes now except pyamrex.