Bug 124799 - osl_getEnvironment wrapper ...
Summary: osl_getEnvironment wrapper ...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
6.3.0.0.alpha0+
Hardware: All All
: medium minor
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyMedium, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2019-04-17 15:15 UTC by Michael Meeks
Modified: 2023-10-03 18:26 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2019-04-17 15:15:55 UTC
Its unclear if we need this anymore - but the osl_getEnvironment at least returns an OUString which is nice.

However having a pleasant in-line C++ wrapper which would return an empty string if not set; along with an optional default would be much nicer:

git grep -3 osl_getEnvironment

and weep =) ditto for setEnvironment. Something like:

OUString getEnvironment(const OUString &name, const OUString &optionalDefault = OUString());

or whatever would be a nice improvement I think - if that makes sense Stephan ?

Thanks (easy hack).
Comment 1 Stephan Bergmann 2019-04-17 15:35:56 UTC
An include/osl/process.hxx C++ wrapper is curiously missing indeed.  However:

* If we add one, it should probably cover C++ wrappers for all (or at least most) of the include/osl/process.h C functionality.

* Even though that would be inline functions, we would need to get them right "on first shot", because MSVC tends to export even such inline functions from the __declspec(dllexport) DLL (and rely on that from __declspec(dllimport)'ing DLLs).

* osl_getEnvironment returns oslProcessError, the C++ wrapper should probably return such an error code too (if only to be able to distinguish between unset and empty env vars).

* An optionalDefault brings up the question whether it should be returned only for unset or also for empty env vars.  Different callers may have different needs/expectations, and I'm not sure whether such an optional arg would be worth it anyway.
Comment 2 Michael Meeks 2019-04-17 16:00:22 UTC
Fair cop - of course, the perfect is the enemy of the good - so I'd love to get a 1st patch for a new process.hxx and a few wrappers; and grow it out.

a 'git grep osl_getEnvironment' shows that it is ~never used outside sal/ - and UNIX's "getenv()" pattern is quite pretty - modulo the problem of no-NULL 
OUStrings - but even that evaporates when you read the ~dozen call sites =)
Comment 3 Stephan Bergmann 2019-04-18 07:47:30 UTC
(In reply to Michael Meeks from comment #2)
> a 'git grep osl_getEnvironment' shows that it is ~never used outside sal/ -
> and UNIX's "getenv()" pattern is quite pretty - modulo the problem of
> no-NULL 
> OUStrings - but even that evaporates when you read the ~dozen call sites =)

Sorry, I have no idea what you want to say in that paragraph.
Comment 4 Michael Meeks 2019-04-18 10:21:42 UTC
Ah - sorry for not being clear; I'm suggesting a:

git grep osl_getEnvironment

and consider how many uses outside sal/ make a legitimate distinction between NULL and empty environment variables. An:

OUString aWastedLineAndNoFunctionNesting;
if (new_getenv("FOO", aWastedLineAndNoFunctionNesting) == long_enumeration_value)
{
    Do nothing that useful;
}
function_using_env(aWastedLineAndNoFunctionNesting);

I would suggest is prettier as:

function_using_env(new_getenv("FOO"));

But of course - there is always the native C API if people want something more complex.

Then again - using 'getenv()' is the progammers' choice anyway - with 320 hits - an order of magnitude more - presumably because it is easier to use, read, understand etc.. =)

Otherwise amazing point on the MSVC exporting of inlines - that's rather interesting! quite possibly we should put the API somewhere higher up then where it can be simpler and better adapted for our uses-cases (?)