Page MenuHomePhabricator

[libc] Make more of the libc unit testing llvm independent
ClosedPublic

Authored by michaelrj on Nov 17 2020, 3:49 PM.

Details

Summary

This removes the rest of the llvm dependencies from the libc unit test framework.

Diff Detail

Event Timeline

michaelrj created this revision.Nov 17 2020, 3:49 PM
michaelrj requested review of this revision.Nov 17 2020, 3:49 PM
sivachandra added inline comments.Nov 18 2020, 8:26 AM
libc/utils/UnitTest/CMakeLists.txt
5

You should probably do the same in utils/testutils/CMakeLists.txt? I will do another pass and switch these targets to add_library after I move FPUtil's TestHelpers.

libc/utils/testutils/ExecuteFunctionUnix.cpp
12

Why do we need cstring?

michaelrj updated this revision to Diff 306222.Nov 18 2020, 2:09 PM
michaelrj marked an inline comment as done.

(NOT QUITE WORKING) get the rest of the llvm dependencies out StreamWrapper and FDReader

my FDReader change isn't quite working yet but I need to go work on something else and I
don't want to forget to upload this

submit comments

libc/utils/UnitTest/CMakeLists.txt
5

I'll do that once I get the rest of the llvm dependencies out of utils/testutils which should be pretty soon after I get this next commit working.

libc/utils/testutils/ExecuteFunctionUnix.cpp
12

because otherwise strsignal depends on it (line 81)

abrachet added inline comments.
libc/utils/testutils/FDReaderUnix.cpp
20–23

Needs brackets and no comment

32–35

const -> constexpr
I don't think you need to justify the chunk size with the comment.

40

Probably left over from testing.

42–43

This loop could maybe be
for (ssize_t bytesRead; (bytesRead = ::read(...)); ) And then remove the bytesRead == 0 check

45

Read will not null terminate the buffer, the input probably won't have a '\0' in it either. I think pipeStr.insert(pipeStr.end(), buffer, bytesRead) or similar is safer.

54

I think pipeStr == inputStr is more clear
And remove the comment below

michaelrj updated this revision to Diff 306525.Nov 19 2020, 1:41 PM
michaelrj marked 7 inline comments as done.

This change finishes the work started in the previous one, all of the tests now pass.
Big thank you to abrachet for the comments.

submit comments

libc/utils/testutils/FDReaderUnix.cpp
20–23

I feel very silly for missing that one. That was why the test was failing immediately.

42–43

I made one small change to your code, bytesRead needs to be signed because read uses negative numbers for errors

54

oops, as I'm sure you guessed I forgot to clean up my code before submitting.

sivachandra accepted this revision.Nov 20 2020, 4:14 PM

I am not an expert with pipes. To my eyes, looks OK.

libc/utils/testutils/FDReaderUnix.cpp
10

Remove this?

This revision is now accepted and ready to land.Nov 20 2020, 4:14 PM
michaelrj updated this revision to Diff 306800.Nov 20 2020, 4:26 PM
michaelrj marked an inline comment as done.

remove commented out include

This revision was landed with ongoing or failed builds.Nov 20 2020, 4:27 PM
This revision was automatically updated to reflect the committed changes.
michaelrj edited the summary of this revision. (Show Details)Nov 20 2020, 4:29 PM