On Tue, 7 Aug 2012, Dimitrios Apostolou wrote:
> Thanks Andreas, hp, Mike, for your comments. Mike I'd appreciate if you
> elaborated on how to speed-up sprint_uw_rev(), I don't think I understood what
> you have in mind.
I just commented on comments and just above the nit-level;
formatting and contents. Still, I believe such nits affects
reviewer interest.
Formatting: two spaces after punctuation at end of sentence, see
other comments. You got it right in most places. Some of the
comments you changed were actually wrong *before*, some of those
you actually corrected.
I can't quote the non-inlined patch, so I'll copy-paste some
lines, adding quote-style:
> +/* Return a statically allocated string with the decimal representation of
> + VALUE. String IS NOT null-terminated. */
Write as:
+/* Return a statically allocated string with the decimal representation of
+ VALUE. String IS NOT null-terminated. */
Avoid comments that look like commented out code. Add a word or
two to avoid that.
> + /* Emit the .loc directive understood by GNU as. Equivalent: */
> + /* printf ("\t.loc %u %u 0 is_stmt %u discriminator %u",
> + file_num, line, is_stmt, discriminator); */
Write as:
+ /* Emit the .loc directive understood by GNU as. The following is
+ equivalent to printf ("\t.loc %u %u 0 is_stmt %u discriminator %u",
+ file_num, line, file_num, line, is_stmt, discriminator); */
Most of the new comments in default_elf_asm_output_ascii are
wrongly formatted. Most are missing end-of-sentence punctuation
and two spaces. Some look odd; complete the sentences or add
ellipsis to continue in the next comment. There's also wrong
terminology both in comments and code, in several places: a '\0'
is NIL, not NULL. Better write as \0 for clarity in comments.
> + /* If short enough */
> + if (((unsigned long) (next_null - s) < ELF_STRING_LIMIT)
> + /* and if it starts with NULL and it is only a
> + single NULL (empty string) */
> + && ((*s != '\0') || (s == next_null)))
> + /* then output as .string */
> + s = default_elf_asm_output_limited_string (f, s);
> + else
> + /* long string or many NULLs, output as .ascii */
Write as (note also the s/next_null/next_nil/):
+ /* If short enough... */
+ if (((unsigned long) (next_nil - s) < ELF_STRING_LIMIT)
+ /* ... and if it starts with \0 and it is only
+ single \0 (an empty string) ... */
+ && ((*s != '\0') || (s == next_null)))
+ /* ... then output as .string. */
+ s = default_elf_asm_output_limited_string (f, s);
+ else
+ /* A long string or many \0, output as .ascii. */
Contents: I missed a note on relative speedups. You need to
guard the changes, or else a valid future cleanup could be to
simplify the code, using library functions, possibly slowing it
down. There's just the word "fast" added in a comment,
reminding of self-adhesive go-fast-stripes for cars and computer
cases. :)
> /* Write a signed HOST_WIDE_INT as decimal to a file, fast. */
But *how* much faster? Something like:
/* Write a signed HOST_WIDE_INT as decimal to a file. At some
time, this was X times faster than fprintf ("%lld", l); */
Right, not the most important review bits. Still, you asked for
elaboration. Oh right, I see that was just for Mike's comments.
Well, anyway, since my comments still apply. :P
brgds, H-P
