[PATCH] D55014: Add a new interceptor for getvfsstat(2) from NetBSD

[Kamil Rytarowski via Phabricator via llvm-commits]

krytarowski marked an inline comment as done.
krytarowski added inline comments.


================
Comment at: test/sanitizer_common/TestCases/NetBSD/getvfsstat.cc:18
+  if ((rv = getvfsstat(NULL, 0, ST_WAIT)) == -1)
+    err(EXIT_FAILURE, "getvfsstat");
+
----------------
krytarowski wrote:
> vitalybuka wrote:
> > krytarowski wrote:
> > > vitalybuka wrote:
> > > > you can simplify the test with simple asserts here
> > > Do you mean: `assert(rv != -1)` ? err(3) is already simple enough so I will keep it.
> > I see couple other tiny reasons, which are not strong enough to block the patch, but maybe you'll still consider them.
> > 
> > The only important lines here are 17 and 25. The rest is just necessary distruction from the test,  so smaller is better.
> > 
> > Also I'd prefer assert report with line number, which can be used for quick jump to location than current printout of "getvfsstat".
> > 
> > "if (!buf)" is totally unneeded as it crash anyway, and we don't to that in other tests.
> > 
> > If you don't CHECK them maybe: printf("Filesystem %d %s %s %s\n", i, buf[i].f_fstypename,   buf[i].f_mntonname, buf[i].f_mntfromname);
> > 
> > and consistency: compiler_rt tests never use err() in cases like this, mostly asserts
> > 
> `err(3`) is a BSD style. I will switch it to assert and land soon.
I still have 2-3 APIs to submit to review and I will start addressing reported comments from review.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55014