[libc-commits] [PATCH] D150678: [libc][test] Improve memory check reporting

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/test/src/string/memory_utils/memory_check_utils.h:111
 bool CheckBcmp(cpp::span<char> span1, cpp::span<char> span2, size_t size) {
+  const auto error = [=](const char *msg, int cmp,
+                         size_t mismatch_index) -> bool {
----------------
gchatelet wrote:
> sivachandra wrote:
> > I did not look enough to see if the logging parts are runtime code or test only code. If it is test only, may be add a comment saying that? If not, then a better approach is to use `LIBC_ASSERT`:  https://github.com/llvm/llvm-project/blob/main/libc/src/__support/libc_assert.h
> > I did not look enough to see if the logging parts are runtime code or test only code. If it is test only, may be add a comment saying that? If not, then a better approach is to use `LIBC_ASSERT`:  https://github.com/llvm/llvm-project/blob/main/libc/src/__support/libc_assert.h
> 
> `testing::tlog` is the logging infrastructure that is used in `LibUnitTest` and this file is used in unit tests only (`libc/*test*/src/string/memory_utils/memory_check_utils.h`) so I think this is OK.  Are you suggesting that I comment all `testing::log` lines in this file?
Sorry, I did not compose my comment correctly. I should have said:

```
... if the logging parts are **for testing runtime code**...
```

What I meant was, if these messages are for catching test failures, then they should be in some `EXPECT_*` as the raw log messages do not come with line numbers. Or, if they are only providing assert like protection, using a real assert like `LIBC_ASSERT` is better because they also come with line numbers. About comments, if you do not want to use `EXPECT_*` macros, add comments why.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150678/new/

https://reviews.llvm.org/D150678