I tried to add parameter to function which one had default set last parameter. I added parameter in definition. I'm not sure if what I got is correct behaviour or not, but I expected something else :/. Simple example. I have defined one function Declaration (header file): void foo( bool param=false ); Definition (cpp file) void MyClass::foo( bool param ) { if (param) ; } Being in definition I add new parameter, for example "bool a" in header of definition of function, so result would looked like this: "void MyClass::foo( bool param, bool a )" "Update declaration signature" pops up and after pressing "Alt+1" I get following modification in header file void foo( bool param = false, bool a = {} /* TODO */ ); This not what I expected. I'm skipping here that formatting have not been met, despite I had defined one, where "Add spaces around operators" wasn't set. Reproducible: Always Steps to Reproduce: 1. Having function with default setting parameter add new one in definition of function 2. Made "Update declaration signature" Actual Results: Together with new parameter is added such string: " = {} /* TODO */". Example: void foo( bool param = false, bool a = {} /* TODO */ ); Expected Results: Default setting of parameter is removed and new parameter added. Example: void foo( bool param, bool a ); and alternatively should be shown warning that "Current setting parameters might be broke due to set default parameter. Do you want to continue?" KDevelop, KDevplatform cloned at 30.12.2016 (at evening) from 5.0 branch.
And the second example (met today): I have declaration of method like in below code: namespace MyNameSpace { .. class MyClass : public AnotherClass { Q_OBJECT public: void init( QSettings *pSettings, QWidget *pParent ); ... Definition looks like this: void MyClass::init( QSettings *pSettings, QWidget *pParent ) { .. } In declaration I insert on end of parameter list following string: ", bool bInvokeDetection=false", and then declaration looks like this: void init( QSettings *pSettings, QWidget *pParent, bool bInvokeDetection=false ); I get helper "Adapt signature: Adapt definition signature" and I select "1". Actual Results: In result in definition file I get following change: void MyNameSpace::MyNameSpace::MyClass::init(QSettings* pSettings, QWidget* pParent, bool bInvokeDetection) // body } As you can see. Definition has been totally broken. Even opening brace disappeared. Skipping that Source Formatter doesn't work (in attachment my setting). Expected Results: In result I expected something like this: void MyClass::init( QSettings *pSettings, QWidget *pParent, bool bInvokeDetection ) { // body } Tested with KDevelop and KDevplatform cloned at February 28th, 2016 after 10pm (branch 5.0).
Created attachment 97806 [details] Settings of "Source Formatter" - tab Other
Below please fine example for the version of KDevelop and KDevPlatform cloned and built from branch 5.0, at 08-07.2016. There is simple function: declaration: void foo( bool param = true ); definition: void MyClass::foo( bool param ) { } In declaration of function I rename param to param2. And now choosing "Update definition signature" from "Adapt Signature" helper breaks declaration. I get code like this: declaration void MyClass::foo ( bool param2 ) ; definition (no change): void MyClass::foo( bool param ) { }
(tested on version 5.2.0) "Adapt signature" called in header breaks declaration and doesn't update parameter in definition, only updates usage of this parameter. Header file void slotUpdateMatchingStatus( MatchingStatus matchStatus ); After invoking "Adapt signature. Update definition signature" void MyClass::slotUpdateMatchingStatus( MatchingStatus eMatchStatus ) ; Definition file: void MyClass::slotUpdateMatchingStatus( FindReplaceTextBar::MatchingStatus matchStatus ) { if ( eMatchStatus == Value1 ) do_someting else if ( eMatchStatus == Value2 ) do_someting }
Thank you for reporting this issue in KDE software. As it has been a while since this issue was reported, can we please ask you to see if you can reproduce the issue with a recent software version? If you can reproduce the issue, please change the status to "REPORTED" when replying. Thank you!
Dear Bug Submitter, This bug has been in NEEDSINFO status with no change for at least 15 days. Please provide the requested information as soon as possible and set the bug status as REPORTED. Due to regular bug tracker maintenance, if the bug is still in NEEDSINFO status with no change in 30 days the bug will be closed as RESOLVED > WORKSFORME due to lack of needed information. For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging If you have already provided the requested information, please mark the bug as REPORTED so that the KDE team knows that the bug is ready to be confirmed. Thank you for helping us make KDE software even better for everyone!
I retested this issue and seems that the problem still persists. I mean in result I got the same as I described above, so: void foo(bool param = false, bool a = {} /* TODO */); Placed in header file. Of course I'm aware that default value must be last in declaration, but seems here KDevelop cannot handle this case correctly and breaks code. Tested with: KDevelop version 5.9.221170 (22.11.70) $ apt show kdevelop Package: kdevelop Version: 4:22.08.1+p22.04+tunstable+git20221012.0016-0 Operating System: KDE neon Unstable Edition KDE Plasma Version: 5.26.80 KDE Frameworks Version: 5.101.0 Qt Version: 5.15.7 Kernel Version: 5.15.0-53-generic (64-bit) Graphics Platform: X11 Graphics Processor: Mesa Intel® Xe Graphics
The `= {} /* TODO */` part is intentional - implemented in https://commits.kde.org/kdevelop/ba2ecbaeba206644c1134569e6da40047096487a to fix another bug you reported :). Locations of the feature and the test in the current code: $ git grep -n '/\* TODO \*/' plugins/clang/codegen/adaptsignatureassistant.cpp:213: newSignature.defaultParams[i] = QStringLiteral("{} /* TODO */"); plugins/clang/tests/test_assistants.cpp:469: << "class Foo { void bar(bool b, int i = 0, char c = {} /* TODO */); };"
(In reply to Igor Kushnir from comment #8) > The `= {} /* TODO */` part is intentional - implemented in > https://commits.kde.org/kdevelop/ba2ecbaeba206644c1134569e6da40047096487a to > fix another bug you reported :). Locations of the feature and the test in > the current code: > $ git grep -n '/\* TODO \*/' > plugins/clang/codegen/adaptsignatureassistant.cpp:213: > newSignature.defaultParams[i] = QStringLiteral("{} /* TODO */"); > plugins/clang/tests/test_assistants.cpp:469: << "class Foo { void > bar(bool b, int i = 0, char c = {} /* TODO */); };" So the fix breaks different use case.
(In reply to Piotr Mierzwinski from comment #9) > So the fix breaks different use case. But why does it break a use case? Your proposed alternative of removing the previous default parameter value can be a loss of information, especially if the code in question has not been put under version control yet. Adding a frequently valid and reasonable new default parameter value does not lose information. The TODO marker suggests the user to consider what the default value should be. If the user prefers no default values whatsoever, (s)he can remove both the new and the old one easily. Again the TODO marker reminds the user about this. I don't think the current behavior is much worse than showing a warning. Showing a warning is probably more difficult to get right (a nested event loop or code scattered across several functions).
(In reply to Igor Kushnir from comment #10) > (In reply to Piotr Mierzwinski from comment #9) > > So the fix breaks different use case. > But why does it break a use case? Your proposed alternative of removing the > previous default parameter value can be a loss of information, especially if > the code in question has not been put under version control yet. Adding a > frequently valid and reasonable new default parameter value does not lose > information. The TODO marker suggests the user to consider what the default > value should be. If the user prefers no default values whatsoever, (s)he can > remove both the new and the old one easily. Again the TODO marker reminds > the user about this. I don't think the current behavior is much worse than > showing a warning. Showing a warning is probably more difficult to get right > (a nested event loop or code scattered across several functions). OK. I wonder that showing warning would be good reason, but if you say that it is probably more difficult to get right then let be right on your side. Your explanation sounds sensible. I will no longer insist that it is a bug.
One more reason why the current behavior is reasonable: removing the default parameter value breaks code that uses the function. Adding a new parameter with a default value keeps the old code working as before. So this second approach is likely to be used more often by developers - to extend the API without sacrificing compatibility with previous versions.
> Showing a warning is probably more difficult to get right (a nested event loop or code scattered across several functions). Actually there *is* an easy way to show a notification in the shell embedded area (the place above the editor tabs where e.g. the "Failed to configure project ..." error appears when there is a syntax error in CMakeLists.txt). This can be done via IUiController::postMessage(), MessageType=Information or MessageType=Positive. But such a message could become annoying once the user grows accustomed to the `= {} /* TODO */` behavior. So I am not sure it *should* be displayed.