This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Remove ASCII lines from comments
ClosedPublic

Authored by JDevlieghere on Apr 10 2019, 5:27 AM.

Details

Summary

A lot of comments in LLDB are surrounded by an ASCII line to delimit the begging and end of the comment.

//----------------------------------------------------------------------
/// The actual (doxygen) comment, sometimes a single line, but usually wrapped 
/// at 80 cols. 
//----------------------------------------------------------------------

Its use is not really consistent across the code base, sometimes the lines are longer, sometimes they are shorter and sometimes they are omitted. Furthermore, it looks kind of weird with the 80 column limit, where the comment actually extends past the line, but not by much. Furthermore, when /// is used for Doxygen comments, it looks particularly odd. And when // is used, it incorrectly gives the impression that it's actually a Doxygen comment.

I assume these lines were added to improve distinguishing between comments and code. However, given that todays editors and IDEs do a great job at highlighting comments, I think it's worth to drop this for the sake of consistency. The alternative is fixing all the inconsistencies, which would create a lot more churn.

Diff Detail

Event Timeline

JDevlieghere created this revision.Apr 10 2019, 5:27 AM
Herald added a reviewer: jfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

LGTM. Thanks for doing this! These lines are really frustrating to maintain when writing documentation. Also this should bring us yet again a bit closer to the LLVM style, so +1 for that.

+1 Following the local coding style was a pain when adding functions in these files. Good step forward.
This covers the majority of occurrences. Leaving unittests unchanged is probably intentional. There's a few more left in tools and other corners for a follow-up.

labath added a subscriber: labath.Apr 10 2019, 6:47 AM

Let's ship it. :)

This revision was not accepted when it landed; it landed in state Needs Review.Apr 10 2019, 1:48 PM
This revision was automatically updated to reflect the committed changes.