[PATCH] D55014: Add a new interceptor for getvfsstat(2) from NetBSD
Kamil Rytarowski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 30 10:53:29 PST 2018
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
More information about the llvm-commits
mailing list