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.
Details
- Reviewers
wojnilowicz tbaumgart - Group Reviewers
KMyMoney - Commits
- R261:c4bcc976d85e: Add abstraction layer for OS specific functions
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.
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.
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)...
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...
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.
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. | |
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. |
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. |
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... |
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 |
kmymoney/misc/platformtools.h | ||
---|---|---|
35 | Actually it's good advice. Please read through |
kmymoney/misc/platformtools.h | ||
---|---|---|
35 | From the link you mention I read:
Based on that, I would argue it is bad advise. So I would simply remove it. |
kmymoney/misc/platformtools.h | ||
---|---|---|
35 | That is what I wrote i.e. "good" and not "bad" :) |
- fixed blanks betw. parenthesis and brace
- renamed getters
- removed const keyword for return type
- added error message to cmake file
Since I don't have write access to the repository - would either of you mind taking care of the merge?
Thanks!