[libc-commits] [PATCH] D157534: [libc] Fix printf %a padding issue

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/test/src/stdio/sprintf_test.cpp:872
+  // The trailing zeroes were previously not counted when calculating the
+  // padding, which caused a high-precision number to get too much padding.
+  written = __llvm_libc::sprintf(buff, "%50.50a", 0x1.0p0);
----------------
sivachandra wrote:
> sivachandra wrote:
> > michaelrj wrote:
> > > sivachandra wrote:
> > > > This comment talking about what was previously happening, and that it was found via fuzzing doesn't help build the context. Instead, can you describe what exact expected behavior is being tested here?
> > > The fuzzing thing is a header, telling you what category this is. If you look earlier in the test there's a couple different categories of test, such as precision or width, that focus on a specific feature. Saying these are found by fuzzing clarifies that these aren't from a specific category, they're just random things found to have problems.
> > There is nothing wrong if you want to say that it was found by fuzzing. But, if you do say it, you should ideally provide a reference. If this patch was a fix to a user bug, you will likely put a link to the bug report to help build context for the reader. An year later, the comment "Found via fuzzing" means almost nothing without a reference.
> > 
> > That said, my comment was more about describing what is being tested here rather than describing what was happening previously. Once that is fixed, the note that the bug was found via fuzzing is up to you.
> Just read the other patch and it seems like you want to treat "found by fuzzing" as a category of tests. I would instead call them "Padding tests" or something like that in this case and view that as a category.
the category these would fall under is "combined tests" since these aren't strictly padding tests or strictly precision tests, both of which are listed above on lines 608 and 631 respectively. The combined tests category is at the end, the section these are already in. I've removed the note that these were found via fuzzing to avoid confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157534