Created attachment 141364 [details] Umbrello C++ const bugs From the Umbrello CODING-STYLE document at item 43 (line 245 in the one I have, may not be up-to-date): " 43.) "const correctness" should be preserved as much as possible. Make all getters const." Question: Has anyone actually tried to do this with the existing Umbrello sources? I did, and I had to change about 400 lines of code in just about every file in the source tree before it was again usable. The problem with const (in-)correctness is that it is somewhat like cancer: Once it has started in some code somewhere, it propagates until the entire code base is unusable if one insists on subsequent code being const-correct. But I get the impression that for some of the Umbrello code, which has obviously been around since the very beginning of the app, this was never an issue. At some point, a well-meaning maintainer decided "Oh yes ... we should require const-correctness in our coding standards". I am a great believer in const correctness. So I have another question: How can we do this? I am posting the document I created with descriptions of all the function signatures in all the files so that people can try to follow up on it. But since I am not a regular contributor to the Umbrello project, and still have issues with Git, etc., I will leave it up to those who can actually navigate "invent.kde.org" and do something useful here.
Thanks for making the attachment, it is helpful. Minor point: > File: codedocument.h > virtual QString getUniqueTag(const QString& prefix = QString()); // const This one can't be made const due to a side effect this the function body, QString CodeDocument::getUniqueTag (const QString& prefix) const { [...] m_lastTagIndex = number; The compiler reports "assignment of member ‘CodeDocument::m_lastTagIndex’ in read-only object".
Git commit 0457a2ebba571932d609e6ac14d145ce573f585c by Oliver Kellogg. Committed on 08/09/2021 at 21:01. Pushed by okellogg into branch 'master'. Const correctness fixes addressing roughly the first half of attachment 141364 [details]. Changes that did not succeed offhand are postponed to a second batch. M +2 -2 umbrello/codegenerators/ada/adawriter.cpp M +2 -2 umbrello/codegenerators/ada/adawriter.h M +26 -20 umbrello/codegenerators/classifiercodedocument.cpp M +16 -13 umbrello/codegenerators/classifiercodedocument.h M +7 -7 umbrello/codegenerators/codedocument.cpp M +6 -6 umbrello/codegenerators/codedocument.h M +2 -2 umbrello/codegenerators/codegenerator.cpp M +2 -2 umbrello/codegenerators/codegenerator.h M +2 -2 umbrello/codegenerators/codeparameter.cpp M +2 -2 umbrello/codegenerators/codeparameter.h M +2 -2 umbrello/codegenerators/cpp/cppcodegenerator.cpp M +2 -2 umbrello/codegenerators/cpp/cppcodegenerator.h M +3 -3 umbrello/codegenerators/cpp/cppmakecodedocument.cpp M +3 -3 umbrello/codegenerators/cpp/cppmakecodedocument.h M +2 -2 umbrello/codegenerators/cpp/cppwriter.cpp M +2 -2 umbrello/codegenerators/cpp/cppwriter.h M +2 -2 umbrello/codegenerators/csharp/csharpwriter.cpp M +2 -2 umbrello/codegenerators/csharp/csharpwriter.h M +3 -3 umbrello/codegenerators/d/dclassifiercodedocument.cpp M +3 -3 umbrello/codegenerators/d/dclassifiercodedocument.h M +2 -2 umbrello/codegenerators/d/dcodegenerator.cpp M +2 -2 umbrello/codegenerators/d/dcodegenerator.h M +2 -2 umbrello/codegenerators/d/dwriter.cpp M +2 -2 umbrello/codegenerators/d/dwriter.h M +2 -2 umbrello/codegenerators/idl/idlwriter.cpp M +3 -3 umbrello/codegenerators/idl/idlwriter.h M +2 -2 umbrello/codegenerators/java/javaantcodedocument.cpp M +2 -2 umbrello/codegenerators/java/javaantcodedocument.h M +4 -4 umbrello/codegenerators/java/javaclassifiercodedocument.cpp M +4 -4 umbrello/codegenerators/java/javaclassifiercodedocument.h M +2 -2 umbrello/codegenerators/java/javacodegenerator.cpp M +2 -2 umbrello/codegenerators/java/javacodegenerator.h M +2 -2 umbrello/codegenerators/java/javawriter.cpp M +2 -2 umbrello/codegenerators/java/javawriter.h M +2 -2 umbrello/codegenerators/pascal/pascalwriter.cpp M +2 -2 umbrello/codegenerators/pascal/pascalwriter.h M +2 -2 umbrello/codegenerators/perl/perlwriter.cpp M +2 -2 umbrello/codegenerators/perl/perlwriter.h M +2 -2 umbrello/codegenerators/python/pythonwriter.cpp M +2 -2 umbrello/codegenerators/python/pythonwriter.h M +4 -4 umbrello/codegenerators/ruby/rubyclassifiercodedocument.cpp M +4 -4 umbrello/codegenerators/ruby/rubyclassifiercodedocument.h M +2 -2 umbrello/codegenerators/sql/mysqlwriter.cpp M +2 -2 umbrello/codegenerators/sql/mysqlwriter.h M +2 -2 umbrello/codegenerators/sql/postgresqlwriter.cpp M +2 -2 umbrello/codegenerators/sql/postgresqlwriter.h M +2 -2 umbrello/codegenerators/sql/sqlwriter.cpp M +2 -2 umbrello/codegenerators/sql/sqlwriter.h M +2 -2 umbrello/codegenerators/vala/valawriter.cpp M +2 -2 umbrello/codegenerators/vala/valawriter.h M +2 -2 umbrello/codeimport/classimport.h M +1 -1 umbrello/debug/debug_utils.cpp M +1 -1 umbrello/debug/debug_utils.h M +2 -2 umbrello/dialogs/activitydialog.h M +2 -2 umbrello/dialogs/codeeditor.h M +4 -4 umbrello/dialogs/finddialog.cpp M +4 -4 umbrello/dialogs/finddialog.h M +2 -2 umbrello/dialogs/objectnodedialog.h M +5 -3 umbrello/dialogs/pages/codegenerationpolicypage.cpp M +2 -2 umbrello/dialogs/settingsdialog.h M +2 -2 umbrello/dialogs/statedialog.h M +3 -3 umbrello/docwindow.cpp M +3 -3 umbrello/docwindow.h M +2 -2 umbrello/petalnode.h M +2 -2 umbrello/uml1model/artifact.cpp M +2 -2 umbrello/uml1model/artifact.h M +1 -1 umbrello/uml1model/attribute.cpp M +3 -2 umbrello/uml1model/attribute.h M +3 -2 umbrello/uml1model/checkconstraint.cpp M +3 -2 umbrello/uml1model/checkconstraint.h M +20 -20 umbrello/uml1model/classifier.cpp M +20 -20 umbrello/uml1model/classifier.h M +3 -2 umbrello/uml1model/classifierlistitem.cpp M +3 -2 umbrello/uml1model/classifierlistitem.h M +2 -2 umbrello/uml1model/component.cpp M +2 -2 umbrello/uml1model/component.h M +3 -3 umbrello/uml1model/entity.cpp M +3 -3 umbrello/uml1model/entity.h M +3 -2 umbrello/uml1model/entityattribute.cpp M +3 -2 umbrello/uml1model/entityattribute.h M +2 -2 umbrello/uml1model/enum.cpp M +2 -2 umbrello/uml1model/enum.h M +3 -2 umbrello/uml1model/enumliteral.cpp M +3 -2 umbrello/uml1model/enumliteral.h M +2 -2 umbrello/uml1model/foreignkeyconstraint.cpp M +3 -2 umbrello/uml1model/foreignkeyconstraint.h M +1 -1 umbrello/uml1model/instance.cpp M +1 -1 umbrello/uml1model/instance.h M +7 -7 umbrello/uml1model/operation.cpp M +8 -7 umbrello/uml1model/operation.h M +8 -8 umbrello/uml1model/package.cpp M +8 -8 umbrello/uml1model/package.h M +3 -2 umbrello/uml1model/template.cpp M +3 -2 umbrello/uml1model/template.h M +9 -9 umbrello/uml1model/umlcanvasobject.cpp M +9 -9 umbrello/uml1model/umlcanvasobject.h M +3 -2 umbrello/uml1model/umlobject.cpp M +31 -31 umbrello/uml1model/umlobject.h M +4 -3 umbrello/uml1model/uniqueconstraint.cpp M +4 -3 umbrello/uml1model/uniqueconstraint.h M +11 -11 umbrello/umlscene.h M +2 -2 umbrello/umlwidgets/seqlinewidget.h M +30 -30 umbrello/umlwidgets/widgetbase.h https://invent.kde.org/sdk/umbrello/commit/0457a2ebba571932d609e6ac14d145ce573f585c
(In reply to Oliver Kellogg from comment #1) > Thanks for making the attachment, it is helpful. > > Minor point: > > File: codedocument.h > > virtual QString getUniqueTag(const QString& prefix = QString()); // const > > This one can't be made const due to a side effect this the function body, > > QString CodeDocument::getUniqueTag (const QString& prefix) const > { > [...] > m_lastTagIndex = number; > > The compiler reports > "assignment of member ‘CodeDocument::m_lastTagIndex’ in read-only object". Thank you, Oliver! As I replied in the umbrello-devel user group (but under another email address, sorry about that), one could declare "CodeDocument::m_lastTagIndex" mutable...this would be the easiest way to fix the compilation error. However, from looking at the implementation code, it appears that this member merely implements some kind of sequence number used to generate the new tag. Shouldn't this rather be made a static member, or have a static "generateSequence()" function? Unless there is only one singleton instance of CodeDocument objects, the danger of having duplicate values seems very real here, because each derived instance will have its own copy of the member. Also, the function is declared virtual ... what if a derived class overrides it and thus cannot update the private member of the base class? I suppose this could be avoided if each class were required to have its own unique prefix which is prepended to the number, but I'm not sure where this is enforced.
Git commit 0372f89d7995a5fbc557e6c6090208bc54950e88 by Oliver Kellogg. Committed on 10/09/2021 at 05:51. Pushed by okellogg into branch 'master'. Fix for "C++ importer does not recognize 'noexcept' keyword" : lib/cppparser/keywords.h - Align INSERT calls as table. Add INSERT("noexcept", Token_noexcept). lib/cppparser/lexer.h - In enum Type add Token_noexcept. lib/cppparser/parser.cpp - In functions skipUntilDeclaration, skipUntilStatement while-loop switch add case Token_noexcept. - In function parseExceptionSpecification handle the case (m_lexer->lookAhead(0) == Token_noexcept). test/import/cxx/const-methods.h - Rename file to const-noexcept-methods.h. test/import/cxx/const-noexcept-methods.h - Rename class to ConstNoexceptMethodClass. - Add NoexceptMethod with trailing noexcept. - Add ConstNoexceptMethod with const noexcept and implementation. Related: bug 338649 M +84 -83 lib/cppparser/keywords.h M +1 -0 lib/cppparser/lexer.h M +15 -2 lib/cppparser/parser.cpp D +0 -3 test/import/cxx/const-methods.h A +5 -0 test/import/cxx/const-noexcept-methods.h * The files marked with a * at the end have a non valid license. Please read: https://community.kde.org/Policies/Licensing_Policy and use the headers which are listed at that page. https://invent.kde.org/sdk/umbrello/commit/0372f89d7995a5fbc557e6c6090208bc54950e88
Ouch. I put the wrong bug number in commit 0372f89, sorry.
Git commit 7a8d1cfb0b562fab051c7ec3b46e6eededee5195 by Oliver Kellogg. Committed on 11/09/2021 at 08:57. Pushed by okellogg into branch 'master'. Address remaining trivial const correctness fixes from attachment 141364 [details]. Suggestions that require code change beyond function signature are postponed to a further batch. M +3 -3 umbrello/dialogs/multipagedialogbase.cpp M +3 -3 umbrello/dialogs/multipagedialogbase.h M +5 -5 umbrello/dotgenerator.cpp M +5 -5 umbrello/dotgenerator.h M +2 -2 umbrello/layoutgenerator.cpp M +2 -2 umbrello/layoutgenerator.h M +2 -2 umbrello/toolbarstateother.cpp M +2 -2 umbrello/toolbarstateother.h M +10 -10 umbrello/uml.cpp M +11 -11 umbrello/uml.h M +2 -1 umbrello/uml1model/classifier.cpp M +2 -1 umbrello/uml1model/classifier.h M +5 -5 umbrello/uml1model/umlcanvasobject.cpp M +5 -6 umbrello/uml1model/umlcanvasobject.h M +32 -32 umbrello/uml1model/umlobject.cpp M +33 -33 umbrello/uml1model/umlobject.h M +25 -25 umbrello/umldoc.cpp M +25 -25 umbrello/umldoc.h M +9 -8 umbrello/umllistview.cpp M +6 -6 umbrello/umllistview.h M +1 -1 umbrello/umllistviewitem.cpp M +2 -2 umbrello/umllistviewitem.h https://invent.kde.org/sdk/umbrello/commit/7a8d1cfb0b562fab051c7ec3b46e6eededee5195
(In reply to Oliver Kellogg from comment #6) > Git commit 7a8d1cfb0b562fab051c7ec3b46e6eededee5195 by Oliver Kellogg. > Committed on 11/09/2021 at 08:57. > Pushed by okellogg into branch 'master'. > > Address remaining trivial const correctness fixes from attachment 141364 [details] > [details]. > Suggestions that require code change beyond function signature are postponed > to a further batch. > > M +3 -3 umbrello/dialogs/multipagedialogbase.cpp (...big snip...) If you could now add const overloads to the following functions starting at line 225 in "umlobject.h", this would undoubtibly help further progress in making other things const correct, i.e.: UMLActor * asUMLActor(); UMLArtifact * asUMLArtifact(); UMLAssociation * asUMLAssociation(); UMLAttribute * asUMLAttribute(); (etc. ... then add these declarations after the group:) const UMLActor * asUMLActor() const; const UMLArtifact * asUMLArtifact() const; const UMLAssociation * asUMLAssociation() const; const UMLAttribute * asUMLAttribute() const; and in umlobject.cpp starting at line 1382: UMLActor * UMLObject::asUMLActor() { return dynamic_cast<UMLActor*>(this); } UMLArtifact * UMLObject::asUMLArtifact() { return dynamic_cast<UMLArtifact*>(this); } UMLAssociation * UMLObject::asUMLAssociation() { return dynamic_cast<UMLAssociation*>(this); } (...) adding the corresponding methods after this group, i.e.: const UMLActor * UMLObject::asUMLActor() const { return dynamic_cast<const UMLActor*>(this); } const UMLArtifact * UMLObject::asUMLArtifact() const { return dynamic_cast<const UMLArtifact*>(this); } const UMLAssociation * UMLObject::asUMLAssociation() const { return dynamic_cast<const UMLAssociation*>(this); } I don't think this would break any existing code?
Git commit 54a365905a971803a7791bedaca7f87d11d4e58f by Oliver Kellogg. Committed on 21/11/2021 at 14:51. Pushed by okellogg into branch 'master'. Address comment #7 by Robert Hairgrove : In uml1model/umlobject.{h,cpp} add const overloads to the asUMLActor() to asUMLUseCase() functions. M +31 -1 umbrello/uml1model/umlobject.cpp M +32 -0 umbrello/uml1model/umlobject.h https://invent.kde.org/sdk/umbrello/commit/54a365905a971803a7791bedaca7f87d11d4e58f
Git commit 33d00b623636603280059f6ba429fae688c6d3cb by Oliver Kellogg. Committed on 04/12/2021 at 18:09. Pushed by okellogg into branch 'master'. Followup to commit 54a3659 - start using the const overloads of the UMLObject::asUMLActor() to asUMLUseCase() functions. Accessory changes: umbrello/codegenerators/ada/adawriter.{h,cpp} umbrello/codegenerators/pascal/pascalwriter.{h,cpp} - At function isOOClass() declare UMLClassifier* argument `const'. umbrello/uml1model/instanceattribute.{h,cpp} - At function toString() add `const' qualifier. umbrello/umlwidgets/entitywidget.cpp - In functions calculateSize() and paint() rename local variable `casted' to `umlEA' (short for UMLEntityAttribute). M +2 -2 umbrello/codegenerators/ada/adawriter.cpp M +1 -1 umbrello/codegenerators/ada/adawriter.h M +9 -9 umbrello/codegenerators/codeclassfield.cpp M +2 -2 umbrello/codegenerators/codeclassfielddeclarationblock.cpp M +3 -3 umbrello/codegenerators/codeparameter.cpp M +2 -2 umbrello/codegenerators/cpp/cppcodeclassfield.cpp M +2 -2 umbrello/codegenerators/cpp/cppheadercodedocument.cpp M +1 -1 umbrello/codegenerators/cpp/cppwriter.cpp M +2 -2 umbrello/codegenerators/d/dcodeclassfield.cpp M +2 -2 umbrello/codegenerators/d/dcodeclassfielddeclarationblock.cpp M +1 -1 umbrello/codegenerators/java/javaantcodedocument.cpp M +2 -2 umbrello/codegenerators/java/javacodeclassfield.cpp M +2 -2 umbrello/codegenerators/java/javacodeclassfielddeclarationblock.cpp M +3 -3 umbrello/codegenerators/ownedcodeblock.cpp M +2 -2 umbrello/codegenerators/ownedhierarchicalcodeblock.cpp M +4 -4 umbrello/codegenerators/pascal/pascalwriter.cpp M +1 -1 umbrello/codegenerators/pascal/pascalwriter.h M +2 -2 umbrello/codegenerators/ruby/rubycodeclassfield.cpp M +2 -2 umbrello/codegenerators/ruby/rubycodeclassfielddeclarationblock.cpp M +4 -4 umbrello/codegenerators/sql/sqlwriter.cpp M +1 -1 umbrello/codeimport/sqlimport.cpp M +4 -4 umbrello/dialogs/codeeditor.cpp M +2 -2 umbrello/dialogs/pages/classgeneralpage.cpp M +4 -4 umbrello/dialogs/pages/classifierlistpage.cpp M +2 -2 umbrello/dialogs/umlattributedialog.cpp M +2 -2 umbrello/dialogs/umlentityattributedialog.cpp M +2 -2 umbrello/dialogs/umlenumliteraldialog.cpp M +3 -3 umbrello/dialogs/umlforeignkeyconstraintdialog.cpp M +2 -2 umbrello/dialogs/umltemplatedialog.cpp M +2 -2 umbrello/dialogs/umluniqueconstraintdialog.cpp M +2 -2 umbrello/dialogs/widgets/defaultvaluewidget.cpp M +2 -2 umbrello/dialogs/widgets/documentationwidget.cpp M +3 -3 umbrello/dialogs/widgets/umldatatypewidget.cpp M +1 -1 umbrello/docwindow.cpp M +1 -1 umbrello/menus/umllistviewpopupmenu.cpp M +4 -4 umbrello/model_utils.cpp M +4 -4 umbrello/models/objectsmodel.cpp M +2 -2 umbrello/uml1model/attribute.cpp M +1 -1 umbrello/uml1model/foreignkeyconstraint.cpp M +1 -1 umbrello/uml1model/instanceattribute.cpp M +1 -1 umbrello/uml1model/instanceattribute.h M +3 -3 umbrello/uml1model/operation.cpp M +4 -4 umbrello/uml1model/uniqueconstraint.cpp M +5 -5 umbrello/umllistview.cpp M +5 -5 umbrello/umllistviewitem.cpp M +10 -10 umbrello/umlscene.cpp M +3 -3 umbrello/umlwidgets/artifactwidget.cpp M +5 -5 umbrello/umlwidgets/associationwidget.cpp M +5 -5 umbrello/umlwidgets/componentwidget.cpp M +12 -12 umbrello/umlwidgets/entitywidget.cpp M +3 -3 umbrello/umlwidgets/enumwidget.cpp M +1 -1 umbrello/umlwidgets/messagewidget.cpp M +1 -1 unittests/testumlobject.cpp https://invent.kde.org/sdk/umbrello/commit/33d00b623636603280059f6ba429fae688c6d3cb