Patch: Align 'for' comments for includes and allow a different line length than 80 chars

[GoogleCodeExporter]

Hi,

I've written a small patch on top of r608 that aligns the `for' comments 
(#include <foo> // for bar).
I've furthermore added a command line flag that gives control over how long the 
output lines can be. Right now you can only have them be limited to 80 chars or 
have no limit at all.

Cheers

Original issue reported on code.google.com by fabian.g...@fadeopolis.com on 2 Apr 2015 at 2:27

Attachments:

• align_comments.diff

[GoogleCodeExporter]

Thanks!

Functionally, this looks great to me, see below for a few smaller things I'd 
like polished:

+    {"max_line_length", optional_argument, NULL, 'l'},

Please also add a help text for this to the usage string in 
iwyu_globals.cc:PrintHelp.

+  int max_line_length; // output lines will be truncated to this length

Please have the comment follow the other ones with short options, something 
like:

  // -l: truncate output lines to this length

+  // reduce max_length to make room for the ", etc." suffix

Start comments with a capital letter and end with a full stop. Also, the suffix 
is ", etc" without a full stop.

+  if (desired_length > max_length)
+    desired_length = max_length;

  desired_length = std::min(desired_length, max_length);

?

+OutputLine PrintableIncludeOrForwardDeclareLine_(
+    const OneIncludeOrForwardDeclareLine& line,
+    const set<string>& associated_quoted_includes) {

Blank line before this declaration. Also, please remove the now-unused 
PrintableIncludeOrForwardDeclareLine function and use that suffix-less name for 
this function.

+  string retval = line.line();
+

This is unused and can be removed.

+    if (it->needs_alignment() &&it->line_length() > line_length)
+      line_length = it->line_length();

I think you could replace needs_alignment() and line_length() with a method on 
OutputLine that does all this, something like:

  size_t align(size_t current_line_length) const {
     if (symbols.empty() || line_.size() <= current_line_length)
        return current_line_length;

     return line_.size();
  }

+  // align lines and produce final output
+  CHECK_((max_line_length >= 0) && "Max line length must be positive");

Put this check just after you initialize max_line_length.

Original comment by kim.gras...@gmail.com on 6 Apr 2015 at 3:00

[GoogleCodeExporter]

Hi,

Thanks for taking the time to look at my patch.
I've created an updated version of it with your suggestions.

The only difference: Instead of adding an `align' method I changed the code for 
computing the required alignment to:

+  for (Each<OutputLine> it(&output_lines); !it.AtEnd(); ++it) {
+    // Only consider lines that need alignment.
+    if (it->needs_alignment())
+      line_length = std::max(it->line_length(), line_length);
+  }

Adding an align method is fine by me too, if you prefer it that way I'll update 
the patch. It's your code base after all ;P

Cheers

Original comment by fabian.g...@fadeopolis.com on 13 Apr 2015 at 2:09

Attachments:

• align_why_comments.diff

[GoogleCodeExporter]

Looks good to me!

There are a few small formatting points I'd do differently, but I can fix those 
before committing if that's OK with you:

+  string printable_line(size_t desired_length, size_t max_length) const;
+ private:
+  string line_;                     // '#include XXX' or 'class YYY;'

Blank line before private.

+  // Compute max width of lines with comments so we can align them nicely.
+  size_t line_length     = 0;

We typically don't align assignments, because renaming vars jumbles the 
formatting.

One functional question:

+  // Reduce max_length to make room for the ", etc" suffix.
+  max_length -= strlen(", etc.");
...
+        retval += ", etc";

You're reducing max_length by len(", etc."), but only adding ", etc" -- the 
period in the first line shouldn't be there, right?

I'd like for vsapsai to take a quick look, but I'm happy with this as-is 
otherwise, and I think it paves the way nicely for #163.

Thanks!

Original comment by kim.gras...@gmail.com on 13 Apr 2015 at 7:46

[GoogleCodeExporter]

Hi,

yes, reformat as needed, I have no problem with that.

Good catch, the extra fullstop in strlen(", etc.") is indeed a bug.

Thanks again for taking the time to review my patch.

Original comment by fabian.g...@fadeopolis.com on 13 Apr 2015 at 8:09

[GoogleCodeExporter]

I have the following questions/comments:

* Do we really need short option "l"?
* Can we mention max_line_length  default value in USAGE?
* There is a waring "iwyu_globals.cc:162:7: warning: field 'pch_in_code' will 
be initialized after field 'max_line_length' [-Wreorder]".
* Can we test this change automatically?

Original comment by vsap...@gmail.com on 20 Apr 2015 at 2:06

[GoogleCodeExporter]

I have updated the patch with your suggestions.

* Do we really need short option "l"?
  I don't really care if there is a short option for this functionality, if you don't like it remove it.

* Can we test this change automatically?
  At least with the default max_line_length the output should be completely unchanged and if I read the reports from the iwyu test script correctly it is. I don't see an easy way to test other max line lengths without hand written tests.

Original comment by fabian.g...@fadeopolis.com on 21 Apr 2015 at 8:41

Attachments:

• align_why_comments.diff

[GoogleCodeExporter]

Your last patch appears unchanged, but I went ahead and did the changes I asked 
for earlier and the ones vsapsai raised:

- Whitespace edits in OutputLine class declaration
- Remove -l short option
- Fix ", etc." bug
- Fix -Wreorder bug
- Update USAGE to mention default value for --max_line_length
- Oops, forgot the alignment of line_length initialization :-) Oh, well.

Committed in r610, thank you very much!

Original comment by kim.gras...@gmail.com on 25 Apr 2015 at 9:23

[GoogleCodeExporter]

Original comment by kim.gras...@gmail.com on 25 Apr 2015 at 9:24

• Changed state: Fixed