Page MenuHomePhabricator

Fix unit tests on Windows: handle env vars with non-ASCII chars.

Authored by chfast on Nov 3 2015, 3:12 AM.



On Windows we have to take UTF16 encoded env vars and convert them to UTF8. This patch fixes CopyEnvironment helper function used by process unit tests.

Diff Detail


Event Timeline

chfast updated this revision to Diff 39041.Nov 3 2015, 3:12 AM
chfast retitled this revision from to Fix unit tests on Windows: handle env vars with non-ASCII chars..
chfast updated this object.
chfast set the repository for this revision to rL LLVM.
chfast added a subscriber: llvm-commits.
yaron.keren added inline comments.
64 ↗(On Diff #39041)

Will this unit test never run multithreaded?

chfast added inline comments.Nov 3 2015, 3:37 AM
64 ↗(On Diff #39041)

That is the weak point. What do you think about extracting storage as a function argument?

yaron.keren added a comment.EditedNov 3 2015, 6:09 AM

I see the usage pattern is CopyEnvironment, push_back something, use std::vector<const char *> with ExecuteAndWait. For this you must have a consecutive vector of char * in memory, so someone will have to manage their lifetime. It's better not to leave this to the caller or split responsibility so you could objectize CopyEnvironment to do everything required:

class CopyEnvironment {
  std::vector<const char *> env;
  CopyEnvironment() { copy environment into newly allocated copies, even on unix  }
  push_back() { env.push_back an allocated copy }
  const char *get() { env.push_back(nullptr); return env[0]; }
  ~CopyEnvironment() { release memory }

so the usage pattern is

CopyEnvironment CE;
ExecuteAndWait(... CE.get()...);

this is not very efficient especially on Unix but for a unittest where CopyEnvironment runs just once per test it should not matter at all.

chfast updated this revision to Diff 39053.Nov 3 2015, 6:13 AM

Use local temporary storage for processing env vars on Windows.

chfast added a comment.Nov 3 2015, 6:22 AM

Sorry, I missed your comment. I have another revision on the way.

chfast updated this revision to Diff 39087.Nov 3 2015, 10:36 AM
chfast removed rL LLVM as the repository for this revision.

Env vars processing is now implemented using GTest Fixture.

yaron.keren accepted this revision.Nov 4 2015, 12:20 AM
yaron.keren added a reviewer: yaron.keren.

Neat and efficient, LGTM

This revision is now accepted and ready to land.Nov 4 2015, 12:20 AM
This revision was automatically updated to reflect the committed changes.