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.