Code review comment for lp://qastaging/~mterry/unity-scopes-api/snap-root

Revision history for this message
Michi Henning (michihenning) wrote :

Could you re-target this MR at the devel branch instead of trunk please? That allows us to use our normal staging process.

Is $SNAP always going to have a trailing slash? If not, constructs such as

$SNAP@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry

and

snap_root + base_dflt_file

won't create the path name correctly. Wouldn't it be safer to append a slash regardless? If we end up with two adjacent slashes, they won't do any harm.

This does not work:

const string snap_root = getenv("SNAP");

If SNAP isn't set, getenv() returns a nullptr, which causes the string constructor to throw. That causes a bunch of tests to fail.

The repeated calls to getenv("SNAP") are a bit of a pain (and dangerous because of the needed check for a nullptr return). It would be better to do this is ConfigBase and and provide a protected accessor for the value. That puts the code to sanitize the value in a single place.

review: Needs Fixing

« Back to merge proposal