[Differential] [Requested Changes To] D4359: Add ClangClassHelper, to restore features of CppClassHelper

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  some nitpicks on the code style, otherwise lgtm

INLINE COMMENTS

> kossebau wrote in clangclasshelper.cpp:137
> No instant idea how to add code sharing without complicating things for the code reader. What did you think of? Be aware that first of 3 loops is over another type of container, which would ask for template code?
> 
> The unrolled code seems more easy to grasp fpr me compared to what I could envision, but eager to learn better code :)

namespace {
  template<typename Map>
  void addVariables(QVariantMap* variables, QLatin1String suffix, const Map& map)
  {
      for (auto it = map.begin(), end = map.end(); it != end; ++it) {
          variables->insert(it.key() + suffix, it.value();
      }
  }
  }

that's easy to grasp and should work, no?

> clangclasshelper.cpp:56
> +    QTemporaryFile file(QDir::tempPath() + QLatin1String("/class_") + name + QLatin1String("_XXXXXX.cpp"));
> +    // TODO: really needed? Qt 5.7 docs claim the file will be removed only on object destruction if "on"
> +    file.setAutoRemove(false);

please try this out now - I also think that you can remove this and the explicit file.remove() further down

> clangclasshelper.cpp:80
> +
> +
> +        if (context && context->childContexts().size() == 1) {

remove extra line

> clangclasshelper.cpp:110
> +    FunctionDescriptionList signalDescriptions;
> +
> +    for (const auto& function : description().methods) {

please introduce

  const auto desc = description();

just to ensure we don't call this more often than required

> clangclasshelper.cpp:123
> +
> +    foreach (const VariableDescription& variable, description().members) {
> +        const QString& access = variable.access.isEmpty() ? publicAccess : variable.access;

don't use foreach:

  for (const auto& variable : description().members) {
      ...
  }

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D4359

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kossebau, mwolff, #kdevelop
Cc: mwolff, arrowdodger, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170202/5bb911b2/attachment.html>