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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libc/test/src/stdio/sprintf_test.cpp:880
+  ASSERT_STREQ_LEN(
+      written, buff,
+      " 0x1.00000000000000000000000000000000000000000000000000p+0");
----------------
Another nit: Add a comment here saying that the output string has a single character padding to make it 58 characters long.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157534