[PATCH] Insert a space at the start of a line comment in case it starts with an alphanumeric character.

[Manuel Klimek]

On Mon, Jun 10, 2013 at 8:58 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Hi klimek,
>
> "//Test" becomes "// Test". This change is aimed to improve code
> readability and conformance to certain coding styles. If a comment starts
> with a
> non-alphanumeric character, the space isn't added, e.g. "//-*-c++-*-" stays
> unchanged.
>
> http://llvm-reviews.chandlerc.com/D949
>
> Files:
>   lib/Format/BreakableToken.cpp
>   lib/Format/BreakableToken.h
>   lib/Format/WhitespaceManager.h
>   unittests/Format/FormatTest.cpp
>
> Index: lib/Format/BreakableToken.cpp
> ===================================================================
> --- lib/Format/BreakableToken.cpp
> +++ lib/Format/BreakableToken.cpp
> @@ -16,6 +16,7 @@
>  #define DEBUG_TYPE "format-token-breaker"
>
>  #include "BreakableToken.h"
> +#include "clang/Basic/CharInfo.h"
>  #include "clang/Format/Format.h"
>  #include "llvm/ADT/STLExtras.h"
>  #include "llvm/Support/Debug.h"
> @@ -164,15 +165,43 @@
>                                             encoding::Encoding Encoding)
>      : BreakableSingleLineToken(Token, StartColumn,
>                                 getLineCommentPrefix(Token.TokenText), "",
> -                               Encoding) {}
> +                               Encoding) {
> +  OriginalPrefix = Prefix;
> +  if (Token.TokenText.size() > Prefix.size() &&
> +      isAlphanumeric(Token.TokenText[Prefix.size()])) {
> +    if (Prefix == "//")
> +      Prefix = "// ";
> +    else if (Prefix == "///")
> +      Prefix = "/// ";
> +  }
> +}
>
>  BreakableToken::Split
>  BreakableLineComment::getSplit(unsigned LineIndex, unsigned TailOffset,
>                                 unsigned ColumnLimit) const {
>    return getCommentSplit(Line.substr(TailOffset), StartColumn +
> Prefix.size(),
>                           ColumnLimit, Encoding);
>  }
>
> +void BreakableLineComment::insertBreak(unsigned LineIndex, unsigned
> TailOffset,
> +                                       Split Split, bool InPPDirective,
> +                                       WhitespaceManager &Whitespaces) {
> +  Whitespaces.breakToken(Tok, OriginalPrefix.size() + TailOffset +
> Split.first,
> +                         Split.second, Postfix, Prefix, InPPDirective,
> +                         StartColumn);
> +}
> +
> +void
> +BreakableLineComment::replaceWhitespaceBefore(unsigned LineIndex,
> +                                              unsigned InPPDirective,
> +                                              WhitespaceManager
> &Whitespaces) {
> +  if (OriginalPrefix != Prefix) {
> +    SourceLocation Loc =
> +
>  Tok.getStartOfNonWhitespace().getLocWithOffset(OriginalPrefix.size());
> +    Whitespaces.storeReplacement(SourceRange(Loc, Loc), " ");
> +  }
> +}
> +
>  BreakableBlockComment::BreakableBlockComment(
>      const FormatStyle &Style, const FormatToken &Token, unsigned
> StartColumn,
>      unsigned OriginalStartColumn, bool FirstInLine, encoding::Encoding
> Encoding)
> Index: lib/Format/BreakableToken.h
> ===================================================================
> --- lib/Format/BreakableToken.h
> +++ lib/Format/BreakableToken.h
> @@ -116,6 +116,9 @@
>  };
>
>  class BreakableLineComment : public BreakableSingleLineToken {
> +  // The prefix without an additional space if one was added.
> +  StringRef OriginalPrefix;
> +
>  public:
>    /// \brief Creates a breakable token for a line comment.
>    ///
> @@ -126,6 +129,11 @@
>
>    virtual Split getSplit(unsigned LineIndex, unsigned TailOffset,
>                           unsigned ColumnLimit) const;
> +  virtual void insertBreak(unsigned LineIndex, unsigned TailOffset, Split
> Split,
> +                           bool InPPDirective, WhitespaceManager
> &Whitespaces);
> +  virtual void replaceWhitespaceBefore(unsigned LineIndex,
> +                                       unsigned InPPDirective,
> +                                       WhitespaceManager &Whitespaces);
>  };
>
>  class BreakableBlockComment : public BreakableToken {
> Index: lib/Format/WhitespaceManager.h
> ===================================================================
> --- lib/Format/WhitespaceManager.h
> +++ lib/Format/WhitespaceManager.h
> @@ -67,6 +67,9 @@
>    /// \brief Returns all the \c Replacements created during formatting.
>    const tooling::Replacements &generateReplacements();
>
> +  /// \brief Stores \p Text as the replacement for the whitespace in \p
> Range.
> +  void storeReplacement(const SourceRange &Range, StringRef Text);
> +
>

The plan is to only generate replacements in one place. I'd rename
breakToken with replaceWhitespaceInToken and add the Newlines parameter
there...


>  private:
>    /// \brief Represents a change before a token, a break inside a token,
>    /// or the layout of an unchanged token (or whitespace within).
> @@ -149,8 +152,6 @@
>    /// \brief Fill \c Replaces with the replacements for all effective
> changes.
>    void generateChanges();
>
> -  /// \brief Stores \p Text as the replacement for the whitespace in \p
> Range.
> -  void storeReplacement(const SourceRange &Range, StringRef Text);
>    std::string getNewLineText(unsigned NewLines, unsigned Spaces);
>    std::string getNewLineText(unsigned NewLines, unsigned Spaces,
>                               unsigned PreviousEndOfTokenColumn,
> Index: unittests/Format/FormatTest.cpp
> ===================================================================
> --- unittests/Format/FormatTest.cpp
> +++ unittests/Format/FormatTest.cpp
> @@ -860,10 +860,15 @@
>    EXPECT_EQ("//    Don't_touch_leading_whitespace",
>              format("//    Don't_touch_leading_whitespace",
>                     getLLVMStyleWithColumns(20)));
> -  EXPECT_EQ(
> -      "//Don't add leading\n"
> -      "//whitespace",
> -      format("//Don't add leading whitespace",
> getLLVMStyleWithColumns(20)));
> +  EXPECT_EQ("// Add leading\n"
> +            "// whitespace",
> +            format("//Add leading whitespace",
> getLLVMStyleWithColumns(20)));
> +  EXPECT_EQ("// whitespace", format("//whitespace", getLLVMStyle()));
> +  EXPECT_EQ("// Even if it makes the line exceed the column\n"
> +            "// limit",
> +            format("//Even if it makes the line exceed the column limit",
> +                   getLLVMStyleWithColumns(51)));
> +  EXPECT_EQ("//--But not here", format("//--But not here",
> getLLVMStyle()));
>    EXPECT_EQ("// A comment before\n"
>              "// a macro\n"
>              "// definition\n"
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130611/348b418c/attachment.html>