| mwolff |
| KDevelop |
Based on files languages/cpp/codegen/cppclasshelper.*
as removed by f31d32f49458897c97388a4b8f40f97e5af02f59
Needs "C++" added to values of X-KDevelop-Language for kdevclangsupport.json,
so kdevclang plugin is found when a "C++" language plugin is queried.
Needs someone with clue to complete whatever is done for the variables
passed via the "included_files" key.
Generation via File template dialog provides default methods
for a C++ class again and C++ templates have content again
if special variables for groups of private, protected and public
methods & members are used.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| languages/clang/codegen/clangclasshelper.cpp | ||
|---|---|---|
| 46 | const? | |
| 53 | While QTemporaryFile guarantees that the file name will be unique, how about using name parameter in it? Something like QLatin1String("/") + name + QLatin1String("class_XXXXXX.cpp"). That might ease debugging in some cases. | |
| 54 | Why do we need it? | |
| 95 | This looks const too. | |
| languages/clang/codegen/clangclasshelper.h | ||
| 6 | Proper copyrights. | |
some nitpicks, otherwise lgtm
| languages/clang/codegen/clangclasshelper.cpp | ||
|---|---|---|
| 73 | scope this lock, i.e. free it before removing the file | |
| 101 | hashes, or do you really need this to be sorted? | |
| 110 | wrap | |
| 113 | wrap | |
| 124 | remove line | |
| 125 | instead use auto vit = ... | |
| 126 | and auto vend = ... | |
| 131 | dito | |
| 138 | dito also, these three blocks can share code, please implement that | |
| 192 | super inefficient. check if base starts with public/private/protected + space, if not, append "public " | |
| languages/clang/codegen/clangclasshelper.h | ||
| 34 | remove empty line | |
| 40 | remove spaces in <...> | |
| 75 | const | |
| languages/clang/kdevclangsupport.json | ||
| 57 ↗ | (On Diff #10752) | separate commit, do so directly |
| languages/clang/codegen/clangclasshelper.cpp | ||
|---|---|---|
| 46 | Cannot do, see ICreateClassHelper::createGenerator(QUrl). | |
| 53 | Sounds like a good idea. That technique might be also deployed to other QTemporaryFile usages in KDevelop. | |
| 54 | Good question, that was in the old CppClassHelper code. Will add a TODO for now only, because perhaps this whole block can be dropped and clang directly provides the wanted info about default methods, and I would first spend time on that :) | |
| 138 | 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 :) | |
| 192 | Looking more closely it seems this whole method is not even called from any place, so removing instead. | |
| languages/clang/codegen/clangclasshelper.h | ||
| 6 | I only did s/Cpp/Clang/g in this file, so nothing worth a copyright, no? :) | |
| languages/clang/kdevclangsupport.json | ||
| 57 ↗ | (On Diff #10752) | Okay, will do. |
some nitpicks on the code style, otherwise lgtm
| languages/clang/codegen/clangclasshelper.cpp | ||
|---|---|---|
| 57 | please try this out now - I also think that you can remove this and the explicit file.remove() further down | |
| 81 | remove extra line | |
| 111 | please introduce const auto desc = description(); just to ensure we don't call this more often than required | |
| 124 | don't use foreach: for (const auto& variable : description().members) {
...
} | |
| 138 | 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? | |
update to review feedback
| languages/clang/codegen/clangclasshelper.cpp | ||
|---|---|---|
| 138 | Well, it means when reading the code one has to jump from the place where it is called to the place where the template is called. Picked up your proposal, works fine. Not sure about that naming of the template method, as it overloads a member method, but the namespace prefix :: should make it obvious also to the human reader what is called, no? | |
| languages/clang/codegen/clangclasshelper.cpp | ||
|---|---|---|
| 138 |
where it is called to the place where the template is declared. | |
the change to kdevlcangsupport.json looks unrelated, otherwise lgtm
| languages/clang/kdevclangsupport.json | ||
|---|---|---|
| 57 ↗ | (On Diff #10864) | uhm this looks wrong - it should be both, C and C++ - did you push the change in a separate commit already? |
| languages/clang/kdevclangsupport.json | ||
|---|---|---|
| 57 ↗ | (On Diff #10864) | As discussed on irc, slipped back in accidentally, will be handled in separate review. |