clang-format recognizes alternative binary operator keywords

[bobmoretti]

Summary

clang-format does not lex the ISO 646 alternative operator keywords as operators; rather, it interprets them as normal identifiers.

This would result in code such as (a) and (b) getting reformatted as (a)and(b).

As a result of some discussion on cfe-dev, this patch changes the lexer option used by clang-format to interpret these keywords as operators and not identifiers.

[bobmoretti]

NB: Phabricator choked on a diff with UTF8, so I replaced all non-ascii characters in the uploaded diff with spaces (which you will see if you expand the context enough).

[nikola]

I don't know anything about this code but I'd really expect both LS_Cpp03 and LS_Cpp11 to mean C++. And this seems to be the case since LangOpts.CPlusPlus is always set to 1. I don't see the value in having the language kind parameter since LK_Cpp is used for all C family of languages.

My suggestion would be to just set CXXOperatorNames to 1, unless there are plans to handle C and C++ differently. But let's see what Manuel has to say.

[bobmoretti]

I didn't add the LanguageKind parameter initially. However, without checking the LanguageKind parameter, a protocol buffers test fails.

Specifically, the and in the following test (FormatTestProto.cpp line 54):

verifyFormat("message SomeMessage {\n"
             "  optional really.really.long.and.qualified.type.aaaaaaa\n"
             "      fiiiiiiiiiiiiiiiiiiiiiiiiield = 1;\n"
             "  optional\n"
             "      really.really.long.and.qualified.type.aaaaaaa.aaaaaaaa\n"
             "          another_fiiiiiiiiiiiiiiiiiiiiield = 2;\n"
             "}");

The and gets misrepresented as an identifier, and the reformatter inserts a space before .qualified.

I don't know the first thing about protocol buffers. Is and a valid type identifier?

If it's not, I'd be happy to replace it in the test, and remove the LanguageKind parameter from getFormattingLangOpts.

[nikola]

Theoretically yes but this is true for any C++ keyword as proto compiler turns message fields into data members with decorated names (appends underscore).

I'd say that this test is broken. If clang format is supposed to format proto files the same way it does C++ files than proto definitions can't use C++ tokens as field names. I'd say that this test needs to be modified. I think the same would happen if test declared message field like this 'optional int32 class'?

[bobmoretti]

Yes, I think that would be a problem. However, this particular failing test case doesn't involve the field name; rather it involves the field's type. So I think a closer parallel would be

message class {
    optional int32 foo = 1;
}

message bar {
    optional class c = 1;
}

How would the proto compiler handle that?

[nikola]

LOL, nope. Generates class class.

So to summarize, protbuf compiler is happy to generate invalid C++ code, and the author of proto definition should take care abot this. It just happens that 'and' is not always treated as a token the way 'class' is. I vote for changing the test but let's hear from Manuel first.

[bobmoretti]

All right, so probably the proto test should not try to use a C++ reserved keyword for a type name. I will change that, and get rid of the LanguageKind parameter.

Getting a little off topic, I'm a little surprised that the same exact lexer is used for all of C, C++, ObjC, and javascript, but I am very naïve about such things. Was it an explicit design decision made previously in clang-format's history?

[bobmoretti]

Oh, I missed your edit. I will wait for more feedback before changing.

Thanks for the info.

[nikola]

This is clang's lexer and as far as I know it was designed to handle all C languages from the start. Not sure how Javascript an Protobuf happened but I'm guessing this was added with clang-format.

[djasper]

I'd be fine with recognizing these as keywords for any language (and changing the stupid proto test).

Otherwise looks good. Do you have commit access?

[bobmoretti]

I updated the proto test to not include any c++ reserved keywords, and I changed the LangOpts returned by getFormattingLangOpts to set CXXOperatorNames to 1 unconditionally, as per review feedback

I do not have commit access.

[nikola]

Done r208269. Thanks for working on this.

include/clang/Format/Format.h

 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                std::vector<tooling::Range> Ranges,
                                StringRef FileName = "<stdin>");
 
 /// \brief Returns the \c LangOpts that the formatter expects you to set.
 ///
 /// \param Standard determines lexing mode: LC_Cpp11 and LS_Auto turn on C++11
 /// lexing mode, LS_Cpp03 - C++03 mode.
-LangOptions getFormattingLangOpts(FormatStyle::LanguageStandard Standard =
-                                      FormatStyle::LS_Cpp11);
+LangOptions getFormattingLangOpts(
+    FormatStyle::LanguageStandard Standard = FormatStyle::LS_Cpp11);
 
 /// \brief Description to be used for help text for a llvm::cl option for
 /// specifying format style. The description is closely related to the operation
 /// of getStyle().
 extern const char *StyleOptionHelpDescription;
 
 /// \brief Construct a FormatStyle based on \c StyleName.
 ///

lib/Format/Format.cpp

 }
 
 LangOptions getFormattingLangOpts(FormatStyle::LanguageStandard Standard) {
   LangOptions LangOpts;
   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.CPlusPlus1y = Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.LineComment = 1;
+  LangOpts.CXXOperatorNames = 1;
   LangOpts.Bool = 1;
   LangOpts.ObjC1 = 1;
   LangOpts.ObjC2 = 1;
   return LangOpts;
 }
 
 const char *StyleOptionHelpDescription =
     "Coding style, currently supports:\n"

tools/clang-format/ClangFormat.cpp

static bool format(StringRef FileName) {
   } else {
     Rewriter Rewrite(Sources, LangOptions());
     tooling::applyAllReplacements(Replaces, Rewrite);
     if (Inplace) {
       if (Rewrite.overwriteChangedFiles())
         return true;
     } else {
       if (Cursor.getNumOccurrences() != 0)
-        outs() << "{ \"Cursor\": " << tooling::shiftedCodePosition(
-                                          Replaces, Cursor) << " }\n";
+        outs() << "{ \"Cursor\": "
+               << tooling::shiftedCodePosition(Replaces, Cursor) << " }\n";
       Rewrite.getEditBuffer(ID).write(outs());
     }
   }
   return false;
 }
 
 }  // namespace format
 }  // namespace clang

unittests/Format/FormatTest.cpp

EXPECT_EQ(
       "  int a; // <- Should not be formatted",
       format(
           "template<typename T>\n"
           "T *getFETokenInfo() const { return static_cast<T*>(FETokenInfo); }\n"
           "  int a; // <- Should not be formatted",
           9, 5, getLLVMStyle()));
 }
 
+TEST_F(FormatTest, RecognizesBinaryOperatorKeywords) {
+    verifyFormat("x = (a) and (b);");
+    verifyFormat("x = (a) or (b);");
+    verifyFormat("x = (a) bitand (b);");
+    verifyFormat("x = (a) bitor (b);");
+    verifyFormat("x = (a) not_eq (b);");
+    verifyFormat("x = (a) and_eq (b);");
+    verifyFormat("x = (a) or_eq (b);");
+    verifyFormat("x = (a) xor (b);");
+}
+
 //===----------------------------------------------------------------------===//
 // Tests for control statements.
 //===----------------------------------------------------------------------===//
 
 TEST_F(FormatTest, FormatIfWithoutCompoundStatement) {
   verifyFormat("if (true)\n  f();\ng();");
   verifyFormat("if (a)\n  if (b)\n    if (c)\n      g();\nh();");
   verifyFormat("if (a)\n  if (b) {\n    f();\n  }\ng();");
verifyFormat("int a = bbbb && ccc && fffff(\n"
                "                           ddd);");
 }
 
 TEST_F(FormatTest, LineBreakingInBinaryExpressions) {
   verifyFormat(
       "bool aaaaaaa =\n"
       "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaa).aaaaaaaaaaaaaaaaaaa() ||\n"
       "    bbbbbbbb();");
+  verifyFormat(
+      "bool aaaaaaa =\n"
+      "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaa).aaaaaaaaaaaaaaaaaaa() or\n"
+      "    bbbbbbbb();");
+  
   verifyFormat("bool aaaaaaaaaaaaaaaaaaaaa =\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbb &&\n"
                "    ccccccccc == ddddddddddd;");
+  verifyFormat("bool aaaaaaaaaaaaaaaaaaaaa =\n"
+               "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa != bbbbbbbbbbbbbbbbbb and\n"
+               "    ccccccccc == ddddddddddd;");
+  verifyFormat(
+      "bool aaaaaaaaaaaaaaaaaaaaa =\n"
+      "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa not_eq bbbbbbbbbbbbbbbbbb and\n"
+      "    ccccccccc == ddddddddddd;");
 
   verifyFormat("aaaaaa = aaaaaaa(aaaaaaa, // break\n"
                "                 aaaaaa) &&\n"
                "         bbbbbb && cccccc;");
   verifyFormat("aaaaaa = aaaaaaa(aaaaaaa, // break\n"
                "                 aaaaaa) >>\n"
                "         bbbbbb;");
   verifyFormat("Whitespaces.addUntouchableComment(\n"
TEST_F(FormatTest, FormatsBuilderPattern) {
   verifyFormat("FirstToken->WhitespaceRange.getBegin().getLocWithOffset(\n"
                "    First->LastNewlineOffset);");
 }
 
 TEST_F(FormatTest, BreaksAccordingToOperatorPrecedence) {
   verifyFormat(
       "if (aaaaaaaaaaaaaaaaaaaaaaaaa ||\n"
       "    bbbbbbbbbbbbbbbbbbbbbbbbb && ccccccccccccccccccccccccc) {\n}");
+  verifyFormat(
+      "if (aaaaaaaaaaaaaaaaaaaaaaaaa or\n"
+      "    bbbbbbbbbbbbbbbbbbbbbbbbb and cccccccccccccccccccccccc) {\n}");
+
   verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaa && bbbbbbbbbbbbbbbbbbbbbbbbb ||\n"
                "    ccccccccccccccccccccccccc) {\n}");
+  verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaa and bbbbbbbbbbbbbbbbbbbbbbbb or\n"
+               "    ccccccccccccccccccccccccc) {\n}");
+
   verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaa || bbbbbbbbbbbbbbbbbbbbbbbbb ||\n"
                "    ccccccccccccccccccccccccc) {\n}");
+  verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaa or bbbbbbbbbbbbbbbbbbbbbbbbb or\n"
+               "    ccccccccccccccccccccccccc) {\n}");
+
   verifyFormat(
       "if ((aaaaaaaaaaaaaaaaaaaaaaaaa || bbbbbbbbbbbbbbbbbbbbbbbbb) &&\n"
       "    ccccccccccccccccccccccccc) {\n}");
+  verifyFormat(
+      "if ((aaaaaaaaaaaaaaaaaaaaaaaaa or bbbbbbbbbbbbbbbbbbbbbbbbb) and\n"
+      "    ccccccccccccccccccccccccc) {\n}");
+
   verifyFormat("return aaaa & AAAAAAAAAAAAAAAAAAAAAAAAAAAAA ||\n"
                "       bbbb & BBBBBBBBBBBBBBBBBBBBBBBBBBBBB ||\n"
                "       cccc & CCCCCCCCCCCCCCCCCCCCCCCCCC ||\n"
                "       dddd & DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD;");
+  verifyFormat("return aaaa & AAAAAAAAAAAAAAAAAAAAAAAAAAAAA or\n"
+               "       bbbb & BBBBBBBBBBBBBBBBBBBBBBBBBBBBB or\n"
+               "       cccc & CCCCCCCCCCCCCCCCCCCCCCCCCC or\n"
+               "       dddd & DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDD;");
+
   verifyFormat("if ((aaaaaaaaaa != aaaaaaaaaaaaaaa ||\n"
                "     aaaaaaaaaaaaaaaaaaaaaaaa() >= aaaaaaaaaaaaaaaaaaaa) &&\n"
                "    aaaaaaaaaaaaaaa != aa) {\n}");
+  verifyFormat("if ((aaaaaaaaaa != aaaaaaaaaaaaaaa or\n"
+               "     aaaaaaaaaaaaaaaaaaaaaaaa() >= aaaaaaaaaaaaaaaaaaaa) and\n"
+               "    aaaaaaaaaaaaaaa != aa) {\n}");
 }
 
 TEST_F(FormatTest, BreaksAfterAssignments) {
   verifyFormat(
       "unsigned Cost =\n"
       "    TTI.getMemoryOpCost(I->getOpcode(), VectorTy, SI->getAlignment(),\n"
       "                        SI->getPointerAddressSpaceee());\n");
   verifyFormat(

unittests/Format/FormatTestProto.cpp

verifyFormat("message SomeMessage {\n"
                "  required int32 field1 = 1;\n"
                "}");
   verifyFormat("message SomeMessage {\n"
                "  required int32 field1 = 1;\n"
                "  optional string field2 = 2 [default = \"2\"]\n"
                "}");
 
   verifyFormat("message SomeMessage {\n"
-               "  optional really.really.long.and.qualified.type.aaaaaaa\n"
+               "  optional really.really.long.qualified.type.aaa.aaaaaaa\n"
                "      fiiiiiiiiiiiiiiiiiiiiiiiiield = 1;\n"
                "  optional\n"
-               "      really.really.long.and.qualified.type.aaaaaaa.aaaaaaaa\n"
+               "      really.really.long.qualified.type.aaa.aaaaaaa.aaaaaaaa\n"
                "          another_fiiiiiiiiiiiiiiiiiiiiield = 2;\n"
                "}");
 }
 
 TEST_F(FormatTestProto, FormatsEnums) {
   verifyFormat("enum Type {\n"
                "  UNKNOWN = 0;\n"
                "  TYPE_A = 1;\n"