Add QMetaEnum usage for autogenerated code#317
Add QMetaEnum usage for autogenerated code#317kunichik wants to merge 1 commit intoPelagicore:masterfrom
Conversation
|
|
||
| namespace facelift { | ||
|
|
||
| namespace Enum { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/model/FaceliftEnum.cpp
Outdated
| namespace Enum { | ||
|
|
||
| void onAssignFromStringError(const QString &s) | ||
| void fromStringError(const QString &string) |
There was a problem hiding this comment.
bad function name. Either the function name indicates what the function does (with a verb), or it indicates when it is called (onXXXXX).
There was a problem hiding this comment.
agree!
how about raiseFatalError ?
| } | ||
| }; | ||
|
|
||
| template<typename ElementType> |
There was a problem hiding this comment.
Why are you changing those names ??
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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) |
There was a problem hiding this comment.
You are causing some data to be allocated on the heap, which is definitely not needed to convert a string to an enum.
There was a problem hiding this comment.
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
3 participants
No description provided.