- User Since
- May 26 2014, 12:49 PM (147 w, 5 d)
Wed, Mar 22
I think we need something before you commit, because as it is people will be specifying --color-output unnecessarily. Even if you say "default = on" or "default = best-effort"
It's hard for me to review on correctness since I'm not familiar with these APIs (and I don't think anyone else is either) but from a functionality standpoint I think these are great changes and I don't see anything glaringly wrong, so lgtm unless someone else has objections.
Tue, Mar 21
The architecture help change snuck in, I already removed it locally but didn't re-upload.
- Fixed issue with returning by value from operator*.
- Moved the test from Support to ADT, since there was already an existing StringMapTest class there that I hadn't noticed.
- Removed the join changes that had snuck in. I'll commit those separately.
Mon, Mar 20
Forgot to add llvm-commits, will need to re-create
See what you think about this. I've created a folder called Mocks under Utility, and created a new target out of it. UtilityTests links against it, and so does InterpreterTests. To do this I had to add the lldb project root as an include directory, this way you can write #include "unittests/Utility/Mocks/MockTildeExpressionResolver.h". Another possibility would be to create lldb/unittests/Mocks/Mocks/Utility, and then add lldb/unittests/Mocks as an include folder. This way you could write #include "Mocks/Utility/MockTildeExpressionResolver.h". I don't have a strong preference either way.
Sun, Mar 19
Turns out ResolveUsername was only being called from one place outside of FileSpec and ResolvePartialUsername was dead code (since callers had already been updated to use TildeExpressionResolver. So I just deleted these two functions and replaced the calls to ResolveUsername with calls directly into TildeExpressionResolver.
Sat, Mar 18
Fri, Mar 17
Forgot to remove Stat declaration from header file.
That one is calling a file static function MakeDirectory, not FileSystem::MakeDirectory, and the implementation of that function already calls llvm::sys::fs::create_directories() to create the whole tree, so it should be fine.
Messed up reviewer / subscriber.
In the places where you want to read from an ifstream and write to a socket, you might consider using llvm::sys::fs::copy_file, declared in Support/FileSystem.h. Currently it takes two paths, but all it does is call openFileForRead() on the first one and openFileForWrite() on the second one to get FDs. So you could probably add an overload that takes two FDs, and have the path version just call the FD version. Then you could call the FD version directly with an FD for your file and an FD for your socket.
Thu, Mar 16
lgtm, do you have commit access?
Looks good with one more suggested fix.