[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[C. Rayroud via Phabricator via cfe-commits]

crayroud added a comment.

I do not have commit access, could you please help with the push.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[C. Rayroud via Phabricator via cfe-commits]

crayroud added a comment.

I do not have commit access, could you please help with the push.

Here are the commit details: C. Rayroud 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[C. Rayroud via Phabricator via cfe-commits]

crayroud added a comment.

Could you please wait before to push this changes to the main branch, there is 
one more point I want to verify.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[C. Rayroud via Phabricator via cfe-commits]

crayroud added a comment.

I do not have commit access, could you please help with the push?

Here are the commit details: C. Rayroud - rayro...@gmail.com


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[C. Rayroud via Phabricator via cfe-commits]

crayroud closed this revision.
crayroud added a comment.

git revision: 6facafe7da59eb02e8c1519670b503efda2f4b58 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[C. Rayroud via Phabricator via cfe-commits]

crayroud added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14165
+  SpaceFuncDecl.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  SpaceFuncDecl.SpaceBeforeParensOptions.AfterFunctionDeclarationName = true;
+

MyDeveloperDay wrote:
> are you testing the -ve case anywhere?
What do you mean by "-ve case" ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114696: [clang-format] regressed default behavior for operator parentheses

[C. Rayroud via Phabricator via cfe-commits]

crayroud added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14126-14127
+  // FIXME these tests regressed behaviour.
+  // verifyFormat("T A::operator() ();", Space);
+  // verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);

Why did you comment these lines? When SpaceBeforeParens is set to always it is 
not correct to have a space before the opening parenthesis, for operators ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114696/new/

https://reviews.llvm.org/D114696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114725: [clang-format] add back tests which didn't cause a regression

[C. Rayroud via Phabricator via cfe-commits]

crayroud added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14125
   verifyFormat("typedef void (*cb) (int);", Space);
   // FIXME these tests regressed behaviour.
+  verifyFormat("T A::operator() ();", Space);

Remove the comment too.



Comment at: clang/unittests/Format/FormatTest.cpp:14277
+  // FIXME these tests regressed behaviour.
   // verifyFormat("T A::operator() () {}", SpaceFuncDef);
   verifyFormat("auto lambda = [] () { return 0; };", SpaceFuncDef);

What do you think of enabling this test and to remove the space before the 
opening parenthesis ? As it is now the expected behaviour to have no space.



Comment at: clang/unittests/Format/FormatTest.cpp:14353
+  // FIXME these tests regressed behaviour.
   // verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);

As the the condition is to add a space before non empty parentheses, the test 
is valid.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114725/new/

https://reviews.llvm.org/D114725

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114725: [clang-format] add back tests which didn't cause a regression

[C. Rayroud via Phabricator via cfe-commits]

crayroud added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14277
+  // FIXME these tests regressed behaviour.
   // verifyFormat("T A::operator() () {}", SpaceFuncDef);
   verifyFormat("auto lambda = [] () { return 0; };", SpaceFuncDef);

crayroud wrote:
> What do you think of enabling this test and to remove the space before the 
> opening parenthesis ? As it is now the expected behaviour to have no space.
In https://reviews.llvm.org/D114696, you proposed to add an operator 
overloading option to SpaceBeforeParensOptions. This example: "T A::operator() 
() {}" should be configured from AfterFunctionDefinitionName or operator 
overloading ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114725/new/

https://reviews.llvm.org/D114725

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114725: [clang-format] add back tests which didn't cause a regression

[C. Rayroud via Phabricator via cfe-commits]

crayroud added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14277
+  // FIXME these tests regressed behaviour.
   // verifyFormat("T A::operator() () {}", SpaceFuncDef);
   verifyFormat("auto lambda = [] () { return 0; };", SpaceFuncDef);

MyDeveloperDay wrote:
> crayroud wrote:
> > crayroud wrote:
> > > What do you think of enabling this test and to remove the space before 
> > > the opening parenthesis ? As it is now the expected behaviour to have no 
> > > space.
> > In https://reviews.llvm.org/D114696, you proposed to add an operator 
> > overloading option to SpaceBeforeParensOptions. This example: "T 
> > A::operator() () {}" should be configured from AfterFunctionDefinitionName 
> > or operator overloading ?
> in this circumstance can you explain why you want to treat differently? I 
> can't tell what you intended here was it by design or as a consequence?
> 
> ```
> verifyFormat("T A::operator()();", SpaceFuncDef);
> verifyFormat("T A::operator() () {}", SpaceFuncDef);
> ```
> 
During the refactoring I treated operator overloading as a function definition. 
Is it correct or not ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114725/new/

https://reviews.llvm.org/D114725

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits