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.

Sign up for GitHub

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

Jump to bottom

Add QMetaEnum usage for autogenerated code #317

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add QMetaEnum usage for autogenerated code #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 10 additions & 26 deletions codegen/facelift/templates/Enum.template.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,37 +74,21 @@ template<> inline const std::initializer_list<{{enum.fullyQualifiedCppType}}>& v
return values;
}

template <> inline QString enumToString(const {{enum.fullyQualifiedCppType}}& v)
{
const char* s = "Invalid";
switch(v) {
{% for member in enum.members %}
case {{enum.fullyQualifiedCppType}}::{{member}}:
s = "{{member}}";
break;
{% endfor %}
default:
break;
}
return s;
}

}
} // end namespace facelift


inline void assignFromString(const QString &s, {{enum.fullyQualifiedCppType}}& v)
inline void fromString(const QString& string, {{enum.fullyQualifiedCppType}}& value)
{
{% for member in enum.members %}
if (s == "{{member}}")
v = {{enum.fullyQualifiedCppType}}::{{member}};
else
{% endfor %}
::facelift::onAssignFromStringError(s);
auto valuePointer = facelift::Enum::fromString<{{enum.fullyQualifiedCppType}}>(string);
if(!valuePointer) {
facelift::Enum::raiseFatalError(string);
} else {
value = *valuePointer;
}
}


inline QTextStream &operator <<(QTextStream &outStream, const {{enum.fullyQualifiedCppType}}& f)
inline QTextStream &operator <<(QTextStream& outStream, const {{enum.fullyQualifiedCppType}}& value)
{
outStream << facelift::enumToString(f);
outStream << facelift::Enum::toString(value);
return outStream;
}
9 changes: 0 additions & 9 deletions src/model/FaceliftCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,6 @@ inline const std::initializer_list<Type> &validValues()
return l;
}


template<typename Type>
QString enumToString(const Type &v)
{
Q_UNUSED(v);
static_assert(!std::is_enum<Type>::value, "Missing specialization of enumToString() template");
return "";
}

#ifdef QT_DEBUG
#define faceliftSeriousError qFatal
#else
Expand Down
10 changes: 7 additions & 3 deletions src/model/FaceliftEnum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,14 @@
#include <QDebug>

namespace facelift {

namespace Enum {
kunichik marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

In facelift, namespaces are used to group entities which belong together into a layer. A upper-level layer (such as "facelift") should not have any dependency to a lower layer (such as "facelift::ipc"). Your change is breaking that rule.
Also, if you introduce a "Enum" namespace (whose name is also not right), you should also introduce a "Struct" namespace, but I do not see any benefit in doing so anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but coding standards/guidelines in facelift, also community page, etc. are EMPTY. This means that you cannot expect any coding standards from anyone.

Also separate namespace for utility makes sense and is a common practice in the software world, especially when you do not want to create a class/struct with all static methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In facelift, namespaces are used to group entities which belong together into a layer. A upper-level layer (such as "facelift") should not have any dependency to a lower layer (such as "facelift::ipc"). Your change is breaking that rule.

in this case namespace is used for its straight purpose - name scope.
Any other options like different name, class or structure are worst alternatives

Also, if you introduce a "Enum" namespace (whose name is also not right),

please suggest a better name

you should also introduce a "Struct" namespace, but I do not see any benefit in doing so anyway.

it's not needed at the moment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually StringConversionHandler is not needed at all
and in the future it should be replaced with toString template function
with different specifications and SFINAE principle.
Also @bitmouse suggested the idea to move the enums stuff into a separate folder.


void onAssignFromStringError(const QString &s)
void raiseFatalError(const QString &string)
{
qFatal("No enum value matching string %s", qPrintable(s));
qFatal("No enum value matching string %s", qPrintable(string));
}

}

} // end namespace Enum
} // end namespace facelift
35 changes: 32 additions & 3 deletions src/model/FaceliftEnum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,40 @@
#pragma once

#include "FaceliftCommon.h"
#include <QObject>
#include <QTextStream>
#include <type_traits>
#include <memory>
#include <QString>
#include <QMetaEnum>
#include <QByteArray>

namespace facelift {

void onAssignFromStringError(const QString &s);
namespace Enum {
kunichik marked this conversation as resolved.
Show resolved Hide resolved

void raiseFatalError(const QString &string);

// Returns the string that is used as the name of the given enumeration value,
// or an empty string if value is not defined
template<class T, std::enable_if_t<QtPrivate::IsQEnumHelper<T>::Value, T>* = nullptr>
QString toString(T value)
{
return QMetaEnum::fromType<T>().valueToKey(static_cast<int>(value));
}

// Returns the enumaration value of the given enumeration key, or nullptr if key is not defined.
// TODO change std::unique_ptr to std::optional when it will be possible
template<typename T>
std::unique_ptr<T> fromString(const QString &string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are causing some data to be allocated on the heap, which is definitely not needed to convert a string to an enum.

Copy link
Collaborator Author

@kunichik kunichik Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I considered that, only std::optional should be here.
and other options like struct, out parameter are worse and
std::unique_ptr will be replaced with std::optional in the future

{
QByteArray byteArray = string.toLocal8Bit();
bool ok = false;
int value = QMetaEnum::fromType<T>().keyToValue(byteArray.data(), &ok);

T result = static_cast<T>(value);

return ok ? std::make_unique<T>(result) : nullptr;
}


} // end namespace Enum
} // end namespace facelift
57 changes: 28 additions & 29 deletions src/model/StringConversionHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,17 @@
#include <QList>
#include <type_traits>
#include "FaceliftCommon.h"
#include "FaceliftEnum.h"

namespace facelift {

class InterfaceBase;
class StructureBase;

template<typename Type, typename Enable = void>
template<typename T, typename Enable = void>
struct StringConversionHandler
{
typedef Type QMLType;

static QString toString(const Type &v)
static QString toString(const T &v)
{
QString s;
QTextStream(&s) << v;
Expand All @@ -65,77 +64,77 @@ struct StringConversionHandler<QVariant>
}
};

QString qObjectToString(const QObject *o);
QString qObjectToString(const QObject *object);

template<typename Type>
struct StringConversionHandler<Type *, typename std::enable_if<std::is_base_of<InterfaceBase, Type>::value>::type>
template<typename T>
struct StringConversionHandler<T*, std::enable_if_t<std::is_base_of<InterfaceBase, T>::value>>
{
static QString toString(const Type *o)
static QString toString(const T *object)
{
return qObjectToString(o);
return qObjectToString(object);
}
};


template<typename Type>
struct StringConversionHandler<Type, typename std::enable_if<std::is_base_of<StructureBase, Type>::value>::type>
template<typename T>
struct StringConversionHandler<T, std::enable_if_t<std::is_base_of<StructureBase, T>::value>>
{
static QString toString(const Type &v)
static QString toString(const T &value)
{
return v.toString();
return value.toString();
}
};


template<typename Type>
struct StringConversionHandler<Type, typename std::enable_if<std::is_enum<Type>::value>::type>
template<typename T>
struct StringConversionHandler<T, std::enable_if_t<QtPrivate::IsQEnumHelper<T>::Value, T>>
{
static QString toString(const Type &v)
static QString toString(T value)
{
return facelift::enumToString(v);
return Enum::toString(value);
}
};

template<typename ElementType>
struct StringConversionHandler<QList<ElementType> >
template<typename T>
struct StringConversionHandler<QList<T> >
{
static QString toString(const QList<ElementType> &v)
static QString toString(const QList<T> &v)
{
QString s;
QTextStream str(&s);
str << "[ ";
for (const auto &element : v) {
str << StringConversionHandler<ElementType>::toString(element);
str << StringConversionHandler<T>::toString(element);
str << ", ";
}
str << "]";
return s;
}
};

template<typename ElementType>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you changing those names ??

Copy link
Collaborator

@bitmouse bitmouse Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know the reason, why changed names, but I like the new version more. It follows general practices in software, in templates, so it should be easier to read for experienced user.

Copy link
Collaborator Author

@kunichik kunichik Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes it's not related to the current changes, but
typename ElementType, typename Type, typename TypeName are bad in most cases and only T is enough here and I just made this small change in current PR.
We need a separate task for replacing non common template parameters names and
hold something like ElementType only for rare cases when it's needed for better readability.

struct StringConversionHandler<QMap<QString, ElementType> >
template<typename T>
struct StringConversionHandler<QMap<QString, T> >
{
static QString toString(const QMap<QString, ElementType> &map)
static QString toString(const QMap<QString, T> &map)
{
QString s;
QTextStream str(&s);
str << "[ ";
for (auto i = map.constBegin(); i != map.constEnd(); ++i) {
str << StringConversionHandler<QString>::toString(i.key());
str << ":";
str << StringConversionHandler<ElementType>::toString(i.value());
str << StringConversionHandler<T>::toString(i.value());
str << ", ";
}
str << "]";
return s;
}
};

template<typename Type>
inline QString toString(const Type &v)
template<typename T>
inline QString toString(const T &v)
{
return StringConversionHandler<Type>::toString(v);
return StringConversionHandler<T>::toString(v);
}


}