sailfishos-applications / filecase

File manager for SailfishOS
Other
9 stars 4 forks source link

[¿Bug?] A couple of compile warnings may denote coding flaws #27

Open Olf0 opened 2 years ago

Olf0 commented 2 years ago
  1. unused parameter
    src/search.cpp: In member function 'void Search::shareFile(const QString&)':
    src/search.cpp:289:39: warning: unused parameter 'file' [-Wunused-parameter]
    void Search::shareFile(const QString &file)
                        ~~~~~~~~~~~~~~~^~~~
  2. unused parameter
    src/dropboxclient.cpp: In member function 'void DropboxClient::moveFile(QString, QString)':
    src/dropboxclient.cpp:697:54: warning: unused parameter 'dest' [-Wunused-parameter]
    void DropboxClient::moveFile(QString source, QString dest)
                                              ~~~~~~~~^~~~
  3. enumeration value 'INVALID' not handled
    src/dropbox/dropbox.cpp: In member function 'QUrl Dropbox::apiToUrl(Dropbox::Api)':
    src/dropbox/dropbox.cpp:35:11: warning: enumeration value 'INVALID' not handled in switch [-Wswitch]
     switch(api)
           ^
  4. unused result
    src/compressedfiles.cpp:92:11: warning: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result [-Wunused-result]
     system(ttt1.toUtf8());
     ~~~~~~^~~~~~~~~~~~~~~
Olf0 commented 11 months ago

@nephros, I am trying to revive FileCase and would appreciate very much, if you look at these.

If the code is too convoluted to comprehend how to resolve one or multiple of these warnings within appropriate time, ultimately "it is just a bunch of warnings".

nephros commented 11 months ago
  1. unused parameter
src/search.cpp: In member function 'void Search::shareFile(const QString&)':
src/search.cpp:289:39: warning: unused parameter 'file' [-Wunused-parameter]
 void Search::shareFile(const QString &file)
                        ~~~~~~~~~~~~~~~^~~~

https://github.com/sailfishos-applications/filecase/blob/devel/src/search.cpp#L289

Well, it's an unused parameter. void Search::shareFile(const QString &file) takes a file argument, but the code does not use it. This is due to the commented-out section in lines 297ff, where it used to be used.

To "fix" that warning you can add some noop that uses the file parameter, like if (file == "") { true } or if 0 { QString dummy = file } or so Q_UNUSED, see comment below.
But really, it's not worth any changes IMO.

nephros commented 11 months ago
2. unused parameter
src/dropboxclient.cpp: In member function 'void DropboxClient::moveFile(QString, QString)':
src/dropboxclient.cpp:697:54: warning: unused parameter 'dest' [-Wunused-parameter]
 void DropboxClient::moveFile(QString source, QString dest)
                                              ~~~~~~~~^~~~

Well, dest is not used, the code always uses d->path and the file name of the source parameter as the destination for the copy operation. It looks like (from using GitHubs "search for this symbol" function") that this is called from nowhere. If that is true, this code could be removed. Alternatively, remove the dest parameter from DropboxClient::moveFile in both the .cpp and the header file.

nephros commented 11 months ago
3. enumeration value 'INVALID' not handled
src/dropbox/dropbox.cpp: In member function 'QUrl Dropbox::apiToUrl(Dropbox::Api)':
src/dropbox/dropbox.cpp:35:11: warning: enumeration value 'INVALID' not handled in switch [-Wswitch]
     switch(api)
           ^

Dropbox::Api is defined here: https://github.com/sailfishos-applications/filecase/blob/b5c44fa9f2e8b867c3eb1abc745eea02defbbeea/src/dropbox/dropbox.h#L37

and includes a value called INVALID.

This is not handled in the switch statement at https://github.com/sailfishos-applications/filecase/blob/b5c44fa9f2e8b867c3eb1abc745eea02defbbeea/src/dropbox/dropbox.cpp#L35

The Warning could be fixed by handling the INVALID case in the switch (maybe log a warning or so). But I haven't studied the code to say whether the omission was intentional.

nephros commented 11 months ago
4. unused result
src/compressedfiles.cpp:92:11: warning: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result [-Wunused-result]
     system(ttt1.toUtf8());
     ~~~~~~^~~~~~~~~~~~~~~

Meh, totally harmless. system returns something which nobody needs.

Trivial fix by doing something like

 (void) system(ttt1.toUtf8());

(I love casting things into the void.)

Although running an external command should probably be handled in a more advanced fashion than just firing the external command and hoping it succeeds.

nephros commented 11 months ago

Addendum to the unused parms:

There is also the Qt Q_UNUSED() macro which can be used to wrap such cases.

Olf0 commented 11 months ago

@nephros, thank you, your feedback is much appreciated!

It was exactly what I wanted, both in form and content:

Consequently I plan to address higher priority tasks first (e.g. Transifex integration), then take care about the two issues I should be able to rectify easily by the help of your guidance and lastly I may come back to you for addressing the final ones (sure not this year).