Fix the design of VampPluginLoader#1282
Fix the design of VampPluginLoader#1282daschuer merged 6 commits intomixxxdj:masterfrom uklotzde:vamp_init
Conversation
| // VAMP_PATH environment variable is already set by the user, then this | ||
| // method appends to that. | ||
| void initPluginPaths() { | ||
| const QLatin1String pathEnv(getenv("VAMP_PATH")); |
There was a problem hiding this comment.
are we sure this is a latin1 string on all targets? I consider this to be a local 8 bit
There was a problem hiding this comment.
Mm... the older implementation created a QString from the const char* returned.
The documentation does not say what it contains, but probably it is locale-dependant:
http://www.cplusplus.com/reference/cstdlib/getenv/
There was a problem hiding this comment.
QString(const char*) isn't any better than QLatin1String(const char*), only less efficient. Maybe we should use fromLocal8Bit() instead?
There was a problem hiding this comment.
yes fromLocal8Bit() should be correct.
There was a problem hiding this comment.
I will also use qgetenv()
| } | ||
| #endif | ||
|
|
||
| QString newPath = pathElements.join(PATH_SEPARATOR); |
There was a problem hiding this comment.
is there a reason to spit an join instead of just append?
There was a problem hiding this comment.
I guess it is in order to avoid implementing the special case of not adding it to the last entry.
There was a problem hiding this comment.
Looks like splitting doesn't make any sense as long as we don't access or replace individual pathElements.
There was a problem hiding this comment.
This was not my focus ;)
|
|
||
| Logger kLogger("VampPluginLoader"); | ||
|
|
||
| Vamp::HostExt::PluginLoader* pPluginLoader = nullptr; |
There was a problem hiding this comment.
I have stumbled over this, because it is hard to distinguish from a local. Can't we use just instance()?
Isn't a name space global the same as a static variable? So we could use the s prefix?
There was a problem hiding this comment.
We could use a 'g' prefix for global (although visibility at the global scope is restricted to this file).
There was a problem hiding this comment.
Thinking about this again, I think "s" prefix can be used here as well.
A anonymous name spaces is the preferred replacement for the multi featured (ambigious) static keyword. So the s can remain even if we fully switch to namespaces.
There was a problem hiding this comment.
I had exactly the same idea of using "s_" and mentioned why in the commit message ;)
|
I will fix the remaining issues to make this code more robust, finally ;) |
|
Revised |
|
CI is done, so I can merge. Thank you for working on it. LGTM |
3 participants
Backported from #1276
Kudos to @JosepMaJAZ for the hint!
@daschuer One minor quick win ;)