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);
 }

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.

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&);

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.

Thank you for the clarification.

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

@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.

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.

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.

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

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.