[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:52:48 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");
+
----------------
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.
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