Smaller code improvements #81

gruenich · 2025-02-25T21:39:08Z

No description provided.

Found by Cppcheck (constVariablePointer).

Found by Cppcheck (missingOverride).

shundhammer · 2025-02-26T10:47:36Z

src/Exception.h

@@ -99,8 +99,8 @@ class NullPointerException: public Exception
 	Exception( "Null pointer" )
 	{}

-    virtual ~NullPointerException() throw()
-	{}
+    virtual ~NullPointerException() throw() override


Why? This override does absolutely nothing; except being in the way and making somebody reading the code think what it might do. The answer is: Nothing.

You cannot override a destructor the wrong way: A destructor cannot have parameters, and there is no return value. And if you try to omit that throw(), the compiler will not complain, with or without override. I even tried it.

So let's not keep adding stuff that only distracts.

shundhammer · 2025-02-26T10:48:21Z

src/Exception.h

@@ -115,8 +115,8 @@ class OutOfMemoryException: public Exception
 	Exception( "Null pointer" )
 	{}

-    virtual ~OutOfMemoryException() throw()
-	{}
+    virtual ~OutOfMemoryException() throw() override


Same. Plus, all those destructors of those exception types are empty anyway.

shundhammer · 2025-02-26T10:48:38Z

src/Exception.h

@@ -128,8 +128,8 @@ class FileException: public Exception
 	_filename( filename )
 	{}

-    virtual ~FileException() throw()
-	{}
+    virtual ~FileException() throw() override


shundhammer · 2025-02-26T10:48:47Z

src/Exception.h

@@ -148,8 +148,8 @@ class SysCallFailedException: public Exception
 	_resourceName( resourceName )
 	{}

-    virtual ~SysCallFailedException() throw()
-	{}
+    virtual ~SysCallFailedException() throw() override


shundhammer · 2025-02-26T10:48:55Z

src/Exception.h

@@ -177,8 +177,8 @@ class DynamicCastException: public Exception
 	Exception( "dynamic_cast failed; expected: " + expectedType )
 	{}

-    virtual ~DynamicCastException() throw()
-	{}
+    virtual ~DynamicCastException() throw() override


shundhammer · 2025-02-26T10:49:04Z

src/Exception.h

@@ -206,8 +206,8 @@ class IndexOutOfRangeException : public Exception
 	, _validMax( validMax )
 	{}

-    virtual ~IndexOutOfRangeException() throw()
-	{}
+    virtual ~IndexOutOfRangeException() throw() override


shundhammer · 2025-02-26T10:52:04Z

src/Logger.cc

@@ -237,8 +237,8 @@ LogSeverity Logger::logLevel( Logger *logger )

    if ( logger )
        return logger->logLevel();
-    else
-        return LogSeverityVerbose;
+


Why?

This was a conscious decision: The control flow is clear. If there is a logger ( the absolutely normal case), that logger's log level is used, otherwise a fallback value.

There is no way that none of those branches are executed; if it were, and the function would not return a value, the compiler would complain. But with this change, it appears as if the fallback were a normal case, not the exception.

shundhammer · 2025-02-26T10:54:20Z

src/Logger.cc

@@ -415,8 +415,8 @@ QString Logger::userName()

    if ( pw )
        return pw->pw_name;
-    else
-        return QString::number( getuid() );
+


Same: This pretends that the error case (not having a password file entry) were the norm. No, it's not; it's an exceptional case. It's not really an error since any system might have user IDs in use that are not in /etc/passwd, but it's still a very rare case.

And again, the compiler does complain if there is any chance that none of those branches are executed.

shundhammer · 2025-02-26T11:00:50Z

src/Logger.cc

@@ -511,23 +511,19 @@ void Logger::logRotate( const QString & logDir,

        if ( dir.exists( newName ) )
        {
-            bool success = dir.remove( newName );
+            [[maybe_unused]] bool success = dir.remove( newName );


Yikes. I really hate the new syntax thingies they introduced with C++11/13/17/20. They needed to introduce weird things like keywords in [[something]] because everything else could very possibly be in use in existing code, so they had to come up with a whole new dimension of ugliness.

Q_UNUSED() is so much better to read, and it's even shorter. There is no need to exchange the well-known simple Q_UNUSED().

And let's not forget that this is just a very thin wrapper around a plain

(void) myVariable;

which has been around since I don't know when. The difference is that Q_UNUSED( myVariable ) clearly announces the purpose of this expression, and (void) myVariable; leaves too many people reading the code scratching their heads what this is supposed to mean.

I agree that the syntax is ugly. It expresses the intend of the code. The void cast trick was useful to silence compiler warnings in the old C days. Now we have proper tools to express what we actually want to do.

It is not the first time that you prefer clear syntax and I clearer semantics and we both want readable code 😉

shundhammer · 2025-02-26T11:04:05Z

src/PkgTaskListWidget.cc

-    {
-        return QListWidgetItem::operator<( otherListWidgetItem );
-    }
+    return QListWidgetItem::operator<( otherListWidgetItem );


Same as above: This silently implies that the if () branch is the exceptional case while the other one (which was the else branch) is the normal control flow. No, it's not; the else branch is the exceptional case. And again, the compiler will complain if there were any way that neither branch is executed, so the function might not return a value. It's certain that it does.

shundhammer · 2025-02-26T11:05:57Z

src/PkgTaskListWidget.cc

@@ -123,12 +120,13 @@ bool PkgTaskListWidgetItem::sortByInsertionSequence() const
    if ( ! listWidget() )
        return false;

-    PkgTaskListWidget * parentPkgTaskListWidget =
+    const PkgTaskListWidget * parentPkgTaskListWidget =


Why?

This does a dynamic cast to a non-const pointer and then assigns that to a const pointer. This looks like a promise that the underlying object will not be modiefied, which is clearly not the case. This const only adds doubts and confusion.

shundhammer · 2025-02-26T11:07:09Z

src/PkgTaskListWidget.cc

+    {
+        return parentPkgTaskListWidget->sortByInsertionSequence();
+    }
+    return false;


Same as above: The false case is not the norm, it's the absolute exception. If somebody gives this function a base class pointer that cannot be dynamic casted, that would be a bug. The code shouldn't crash in that case, but it's clearly not a normal case.

shundhammer · 2025-02-26T11:08:23Z

Sorry, but no winners this time.

If you want to do something good for the project, it still needs a lot of careful testing with some background know-how how it should behave.

yankov-pt · 2025-03-05T14:34:01Z

Whoa, this stuff reads like somebody blindly started changing lines highlighted by a VScode / clangd linter for no reason..I get it, trying to help but this PR seems to only take away time from maintainers. I am actively looking to carve out some time to help this project, hopefully I'll put out something more worthwhile. Is there a global TODO list somewhere - almost everything on the hackweek page is implemented already, not sure what kind of help is needed at this point.

shundhammer · 2025-03-05T20:10:33Z

Well, there is the issue tracker, but we are down to just two leftover ones (#1 is actually the dev blog) at the time of this writing.

#3 is probably not a big thing, but it needs some research what needs to be done; which libzypp calls to use. The result might be a 3-line change. I hope I can get that done in the next days.

#45 means getting the _( "foo" ) calls (left over from YaST) that are actually xgettext() calls and the Qt QObject::tr( "foo" ) calls from the .ui files together and writing some QTranslator that uses xgettext(); and then integrating it with the CMake setup to extract .pot files from the sources, setting up a Weblate environment and another GitHub repo for the translations (to avoid a gazillion of commits from translators using Weblate getting mixed into the source code commits which is an absolute nightmare), creating a separate OBS project myrlyn-lang and of course creating / adapting the .spec files for both. Lots of non-programming tasks that mean fiddling with a dozen tools that all tend to work against you; a total non-fun thing.

And of course in general careful testing the existing functionality, finding and fixing bugs. There might be hidden landmines from the Qt 6 migration; #79 was just one example (and one that affected me on my laptop which did not contribute to make me like Qt 6). There are likely more that are just waiting for somebody unsuspecting to trigger them.

gruenich · 2025-03-05T21:16:53Z

Whoa, this stuff reads like somebody blindly started changing lines highlighted by a VScode / clangd linter for no reason..

As stated in the commit message, it was Cppcheck. To me, fixing over-eager warnings from compilers and static analyzers helps to highlight the real issues which are reported but get overlooked.

gruenich added 4 commits February 25, 2025 23:14

Make a pointer variable const

ee5e78b

Found by Cppcheck (constVariablePointer).

Add missing override's

5e3e2f8

Found by Cppcheck (missingOverride).

Do not have return in else branch

7517a3a

Replace some Q_UNUSED by C++17's [[maybe_unused]]

6f29d2c

shundhammer reviewed Feb 26, 2025

View reviewed changes

shundhammer closed this Feb 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smaller code improvements #81

Smaller code improvements #81

gruenich commented Feb 25, 2025

shundhammer Feb 26, 2025 •

edited

Loading

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

gruenich Mar 5, 2025

shundhammer Feb 26, 2025 •

edited

Loading

shundhammer Feb 26, 2025

shundhammer Feb 26, 2025

shundhammer commented Feb 26, 2025

yankov-pt commented Mar 5, 2025 •

edited

Loading

shundhammer commented Mar 5, 2025 •

edited

Loading

gruenich commented Mar 5, 2025

Smaller code improvements #81

Smaller code improvements #81

Conversation

gruenich commented Feb 25, 2025

shundhammer Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shundhammer Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shundhammer commented Feb 26, 2025

yankov-pt commented Mar 5, 2025 • edited Loading

shundhammer commented Mar 5, 2025 • edited Loading

gruenich commented Mar 5, 2025

shundhammer Feb 26, 2025 •

edited

Loading

shundhammer Feb 26, 2025 •

edited

Loading

yankov-pt commented Mar 5, 2025 •

edited

Loading

shundhammer commented Mar 5, 2025 •

edited

Loading