remove unused unistd.h includes
ClosedPublic

Authored by mhubner on Oct 22 2017, 8:32 PM.

Details

Summary

The includes do not seem to be used in those source files at all. Since they don't exist in MSVC environments, they furthermore cause kmymoney to build properly in MSVC.
Since unused - just removed them.

Test Plan

recompiled after removing the includes.

Diff Detail

Repository
R261 KMyMoney
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mhubner created this revision.Oct 22 2017, 8:32 PM

How come it compiles on MSVC if unistd.h is included unconditionally in e.g. kselectdatabasedlg.cpp for geteuid?

Compiles here on my openSUSE 42.2 w/o problems.

In case they don't exist in some environments, they shall be guarded by #ifdef HAVE_UNISTD_H in all places where needed.

How come it compiles on MSVC if unistd.h is included unconditionally in e.g. kselectdatabasedlg.cpp for geteuid?

It doesn't. Sorry, I didn't mean to say this is the *only* thing that currently makes compiling with MSVC impossible.
I'm working on getting it solved for geteuid and will be submitting a patch for those files that actually use that include shortly.
In source files where the include is not even used at all it should be safe to remove it completely though (as requested with this review request)...

Compiles here on my openSUSE 42.2 w/o problems.

In case they don't exist in some environments, they shall be guarded by #ifdef HAVE_UNISTD_H in all places where needed.

Is this the preferred way of solving platform specific issues in kmymoney?
I looked into this last night and came up with a solution that moves the critical functions (geteuid, getpwuid, getpid) into an own wrapper/helper library that exists in two flavors (1: gnu version that uses unistd.h; 2: nognu version that uses windows.h and lmcons.h for MSVC environments).
I would then only build the appropriate one by checking HAVE_UNISTD_H etc. in the cmake skript.
To me, this seemed a little cleaner than putting platform specific #ifdef magic directly into the source but I'll be happy to follow any guideline that may exist...

mhubner updated this revision to Diff 21407.Oct 27 2017, 2:25 AM

I updated the patch as described above - moved the platform specific stuff into an own library and linked that conditionally. That takes care of all unistd.h / pwd.h related build issues in MSVC that I am aware of.
As I said, if you guys feel that #ifdef's in the source itself are the better solution, I'll be happy to change that accordingly - just let me know.

wojnilowicz requested changes to this revision.Oct 27 2017, 2:28 PM
wojnilowicz added inline comments.
kmymoney/CMakeLists.txt
77

Shouldn't that go to dialogs module as well?

kmymoney/misc/platformtools.h
23

Please forward-declare QString here and include it in source fille. It adds unneded dependency in here.

25

Better idea is to use namespace. Then there is no need for static keyword.
What do you need class for?

kmymoney/misc/platformtools_gnu.cpp
23

This should be included first.

27

Why aggregate-initialization here? QString isn't aggregate type.

30

Please use QString::fromLatin1. pw_name seems to be char*.

32

Parenthesses are unnecessary here.

This revision now requires changes to proceed.Oct 27 2017, 2:28 PM
tbaumgart added inline comments.Oct 27 2017, 2:47 PM
kmymoney/misc/platformtools_gnu.cpp
28

A simple method would be to extract the user name from the QProcessEnvrionment. See http://doc.qt.io/qt-5/qprocess.html#systemEnvironment for details.

mhubner updated this revision to Diff 21504.Oct 28 2017, 11:21 PM

incorporated review feedback

mhubner marked 7 inline comments as done.Oct 28 2017, 11:27 PM
mhubner added inline comments.
kmymoney/misc/platformtools_gnu.cpp
28

I thought about that - if I see that correctly, this reads the USER environment variable - relying on the assumption that no application touched that for whatever reason and populated it with something that is not actually the username didn't seem like the best idea to me, though...

tbaumgart added inline comments.Oct 29 2017, 7:49 AM
kmymoney/misc/CMakeLists.txt
21

Shouldn't we error out here inside an else() branch with an appropriate error message so that we know that the platform is not supported? Maybe a hint, which files are needed on the various platforms.

kmymoney/misc/platformtools.h
35

What is the benefit of returning a const here? I must be missing something.

35

Nitpick: we name all getters without the leadin 'get'. I think that would make sense here as well for the two methods

kmymoney/misc/platformtools_gnu.cpp
31

Nitpick: please drop a blank between closing paren and brace

kmymoney/misc/platformtools_nognu.cpp
33

Same issue with the blank between paren and brace

wojnilowicz added inline comments.Oct 30 2017, 5:53 PM
kmymoney/misc/platformtools.h
35

Actually it's good advice. Please read through
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-out

tbaumgart added inline comments.Oct 31 2017, 8:00 AM
kmymoney/misc/platformtools.h
35

From the link you mention I read:

The argument for adding const to a return value is that it prevents (very rare) accidental access to a temporary. The argument against is prevents (very frequent) use of move semantics.

Based on that, I would argue it is bad advise. So I would simply remove it.

wojnilowicz added inline comments.Oct 31 2017, 1:50 PM
kmymoney/misc/platformtools.h
35

That is what I wrote i.e. "good" and not "bad" :)
I would simply remove it too.

mhubner updated this revision to Diff 21676.Nov 1 2017, 4:06 AM
  • fixed blanks betw. parenthesis and brace
  • renamed getters
  • removed const keyword for return type
  • added error message to cmake file
mhubner marked 8 inline comments as done.Nov 1 2017, 4:08 AM
wojnilowicz accepted this revision.Nov 1 2017, 6:58 AM

Good code. Please commit.

This revision is now accepted and ready to land.Nov 1 2017, 6:58 AM

Since I don't have write access to the repository - would either of you mind taking care of the merge?

Thanks!

Since I don't have write access to the repository - would either of you mind taking care of the merge?

Thanks!

I'll take care of it.

This revision was automatically updated to reflect the committed changes.