Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let PrintInfo use Info() in process base class. #11288

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

WPK4FEM
Copy link
Contributor

@WPK4FEM WPK4FEM commented Jun 19, 2023

Info() and PrintInfo() contain repeated text strings that made override in derived classes necessary.

This commit changes the PrintInfo() implementation such that it uses Info(). This avoids overrides in derived classes if only its default behavior is wanted.

@WPK4FEM WPK4FEM added the FastPR This Pr is simple and / or has been already tested and the revision should be fast label Jun 19, 2023
@WPK4FEM WPK4FEM requested review from avdg81 and huhaas June 19, 2023 14:26
@WPK4FEM WPK4FEM requested a review from a team as a code owner June 19, 2023 14:26
Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure this does not work reliably across compilers. To my knowledge there is no portable way to get the name of the class
Waiting for comments from @KratosMultiphysics/technical-committee

@avdg81
Copy link
Contributor

avdg81 commented Jun 19, 2023

I am pretty sure this does not work reliably across compilers. To my knowledge there is no portable way to get the name of the class Waiting for comments from @KratosMultiphysics/technical-committee

I agree that different compilers may yield different results. According to cppreference.com:

Returns an implementation defined null-terminated character string containing the name of the type. No guarantees are given; in particular, the returned string can be identical for several types and change between invocations of the same program.

Some implementations (such as MSVC, IBM, Oracle) produce a human-readable type name. Others, most notably gcc and clang, return the mangled name, which is specified by the Itanium C++ ABI. [...]

The intent here was to avoid repeating the class name (in Info and PrintInfo). Depending on how these functions are being used (we assumed debugging is their primary purpose), these implementation differences may be acceptable. We would like to add that the current approach where the name is hard-coded is also not ideal. At present, each and every class that derives from Process must override the Info and PrintInfo member functions, or else they will yield wrong results. Also, when a developer decides to rename a class, he/she must not forget to also update these member functions. That is automatically taken care of when the proposed changes are accepted, which helps to keep derived classes clean, since the base class already provides an adequate implementation (in fact, cleaning up our code is what triggered this change in the first place).

In essence, the question is whether we would like to continue repeating class names (at the cost of more code and the possibility that the information becomes inaccurate at some point in time) or that we would like to have an adequate (but not perfect) solution that works for most practical cases. If there are any usages of Info or PrintInfo that require certain guarantees that we are unaware of, we would like to learn about them. Thanks.

@matekelemen
Copy link
Contributor

matekelemen commented Jun 19, 2023

compiler-dependent behaviour is bad already, but there's just no point in sending mangled names to the user.

It's not a great solution, but you could just write a test in python that scans a module for classes, and catches those whose type name doesn't match up with what PrintInfo returns. The downside is obviously that this only works for classes exposed to python.

Another solution would be to make Info a pure virtual function, but that won't help us with tall class hierarchies.

P.S.: we won't be able to solve this problem reliably until C++ gets reflections, but that won't happen in our lifetime. Better not rely on Info and PrintInfo too much.

To avoid necessity of overriding PrintInfo in derived classes.
@WPK4FEM WPK4FEM changed the title Correct type info in base class, to avoid override in derived classes Let PrintInfo use Info() in process base class. Jun 22, 2023
@WPK4FEM WPK4FEM requested a review from philbucher June 22, 2023 09:53
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is a nice improvement. Could someone from the @KratosMultiphysics/technical-committee please have a look? Thanks.

@roigcarlo
Copy link
Member

roigcarlo commented Jun 23, 2023

I agree that would be usefull but @philbucher and @matekelemen are right. Imo (personal opinion, not as tc) if we want to go ahead with this we should use an utility or a macro that demangles it for the main compilers (gcc, clang, icc) and extract the name using a trick like using __PRETTY_FUNCTION__ over a function with a template arguments of the class we want to query:

template<typename TType>
std::string type_name() { return __PRETTY_FUNCTION__; }

std::string demangle(std::string mangled) {
# compilers ifdefs....
}

std::cout << demangle(type_name<Class>) << std::endl;

@avdg81
Copy link
Contributor

avdg81 commented Jun 23, 2023

I agree that would be usefull but @philbucher and @matekelemen are right. Imo (personal opinion, not as tc) if we want to go ahead with this we should use an utility or a macro that demangles it for the main compilers (gcc, clang, icc) and extract the name using a trick like using __PRETTY_FUNCTION__ over a function with a template arguments of the class we want to query:

template<typename TType>
std::string type_name() { return __PRETTY_FUNCTION__; }

std::string demangle(std::string mangled) {
# compilers ifdefs....
}

std::cout << demangle(type_name<Class>) << std::endl;

Please note that we have taken out the change to Info() since that sparked a lot of debate (which is good by the way). Now we are only proposing to change PrintInfo(). The reason is that with that change in place we can remove a considerable amount of code duplication on our end (which we will do in a separate commit).

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok to me now :)

@WPK4FEM WPK4FEM merged commit 1866070 into master Jun 23, 2023
@WPK4FEM WPK4FEM deleted the geo/avoid-info-override branch June 23, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FastPR This Pr is simple and / or has been already tested and the revision should be fast
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants