clang-format #882

srajangarg · 2016-03-23T09:41:00Z

Adresses #866 . Also fixes the trailing whitespace problem @Sumith1896 .

I will soon link a wiki page for the workflow to be followed.
Edit : Here it is https://github.com/symengine/symengine/wiki/Development-workflow-for-SymEngine

…atting

srajangarg · 2016-03-23T10:34:29Z

@certik @isuruf I think the codecov failure can be ignored for this PR.

Sumith1896 · 2016-03-23T12:08:01Z

@srajangarg Could you also update this https://github.com/symengine/symengine/blob/master/doc/style_guide.md (design guide)? Will also help in reviewing this PR. Lot's of changes here.

Sumith1896 · 2016-03-26T18:38:41Z

@srajangarg I'm going through this PR slowly. Could you tell me tell me if there is maximum character limit for a line? ~~Is it this ColumnLimit: 115?~~ I found a longer line!

Sumith1896 · 2016-03-26T18:40:17Z

symengine/add.cpp

                    return Mul::from_dict(p->second, std::move(d2));
                }
            }
            if (is_a<Pow>(*p->first)) {
                insert(m, rcp_static_cast<const Pow>(p->first)->get_base(),
-                    rcp_static_cast<const Pow>(p->first)->get_exp());
+                       rcp_static_cast<const Pow>(p->first)->get_exp());


Is this suggested by clang-format? What rule is this, so that I can document it.

The next line should start from (?

whenever braking a function call, clang-format will align the arguments properly, i.e. the arguments in the new line will match the start of the arguments in the above line.

GOOD my_func(mqwe, pter, lolqw, acc, cccs) BAD my_func(mssad, pwqe, loadsl, adsa, casd)

Agreed. But how do you decide where exactly to move to next line?

I think clang-format won't mind as long as they are aligned, and below the column limit. For our own sake, we can ask the users to fit as many arguments on the initial line before moving to the next.

Sumith1896 · 2016-03-26T19:28:26Z

@srajangarg Thank you for this patient work. I have left some comments above, could you address them?

From what I understand, what you have taken care of is

Single line if condition

if(something)
    oneline

for loop

for (const auto &p : dict) {

}

Space before and after operators
Single line functions written as

type function { single-line-function-here }

Functions

type functions(args)
{

}

The same for namespaces and classes.

We write type &arg and not type& arg
Same goes for type &function

The code sure is more uniform now. I still have following doubts

At places we use //! and at some // for comments. I think this was due to part doxygen effort.
Sometimes we use std::atanh() some other times ::tanh(). Wouldn't just tanh() work for the current namespace?

@certik @isuruf Could you also go through this, atleast my comments to decide on decisions where there is doubt.

srajangarg · 2016-03-27T10:52:55Z

@Sumith1896

The longest limit of a line is indeed 115, but clang-format gives us another option of PenaltyExcessCharacter, which find the right way to "break" a line. Say a line has 118 characters, it will see that its is not feasible to break the line just because of 3 characters, hence doesn't.

It also checks for trailing whitespaces.

I'm still not sure about comments and how they are being handled here.
Yes I agree about the namespace thing, but that I feel doesn't qualify as a code formatting issue.

srajangarg · 2016-03-27T10:53:44Z

I will address the remaining comments/modifications soon.

nishnik · 2016-03-29T14:25:10Z

.clang-format

+Standard:        Cpp11
+TabWidth:        4
+UseTab:          Never
+


This line is not needed.

srajangarg · 2016-03-29T20:09:31Z

@Sumith1896 @certik @isuruf Can you review this quickly? Everytime master changes, merge conflicts arise!

Sumith1896 · 2016-03-29T20:23:39Z

@srajangarg We'll try to block other PRs before this gets merged. Don't worry.

isuruf · 2016-03-30T05:21:07Z

symengine/add.h


-public:
+    public:


Is this necessary?

I can change it, if needed.

isuruf · 2016-03-30T05:25:04Z

.travis.yml

@@ -129,6 +131,7 @@ matrix:
 install:
  - source bin/install_travis.sh
 script:
+  - bin/travis_clang.sh


This should be run only on one Travis job

How should I do that?

Set an environment variable (eg: TEST_CLANG_FORMAT) to true in one of the jobs and check that the variable is true before calling this script.

…l single line functions except empty functions'

certik · 2016-03-30T15:09:43Z

@srajangarg thanks for adding this. Let me play with this as well. E.g. I don't like for the functions to have { on the same line as the declaration, only if it's a method in the header file (update: looks like it is doing the right thing, I must have been looking at a wrong method)

certik · 2016-03-30T15:34:10Z

@srajangarg, @Sumith1896, here is a plan for this PR that I propose:

Forget about master, just keep working on your branch until we are satisfied. This is an important change, let's get it right.
Then just take the formatting scripts, create a new PR off top of the latest master. Then add a commit that formats the whole tree.

That way it should be painless.

I am now installing llvm-3.7 and clang-3.7 from source and will play with this myself.

certik · 2016-03-30T16:32:46Z

bin/travis_clang.sh

+    	if [ $? -ne 0 ]; then
+    		echo "[!] INCORRECT FORMATTING! $FILE" >&2
+    		RETURN=1
+    	fi


You are mixing tabs and spaces here. Use spaces.

certik · 2016-03-30T16:36:19Z

I would like the column limit to be 80. However, when I changed:

--- a/.clang-format
+++ b/.clang-format
@@ -20,7 +20,7 @@ BreakBeforeBinaryOperators: All
 BreakBeforeBraces: Linux
 BreakBeforeTernaryOperators: true
 BreakConstructorInitializersBeforeComma: false
-ColumnLimit:     130
+ColumnLimit:     80
 ConstructorInitializerAllOnOneLineOrOnePerLine: false
 ConstructorInitializerIndentWidth: 4
 ContinuationIndentWidth: 4

it still has longer lines than 80... I see, one also has to do the following change:

--- a/.clang-format
+++ b/.clang-format
@@ -43,7 +43,7 @@ PenaltyBreakBeforeFirstCallParameter: 19
 PenaltyBreakComment: 22312
 PenaltyBreakFirstLessLess: 120
 PenaltyBreakString: 2123
-PenaltyExcessCharacter: 2
+PenaltyExcessCharacter: 1000000
 PenaltyReturnTypeOnItsOwnLine: 60
 PointerAlignment: Right
 SpaceAfterCStyleCast: false

Now it works as expected.

@srajangarg I suggest to do these two changes.

certik · 2016-03-30T16:51:30Z

Another issue that I found is minor, e.g. this in add.cpp:

                    // We can steal the dictionary:
                    // Cast away const'ness, so that we can move
                    // 'dict_', since
                    // 'p->first' will be destroyed when 'd' is at the
                    // end of
                    // this function, so we "steal" its dict_ to avoid
                    // an
                    // unnecessary copy. We know the refcount_ is one,
                    // so
                    // nobody else is using the Mul except us.

should be this:

                    // We can steal the dictionary: Cast away const'ness, so
                    // that we can move 'dict_', since 'p->first' will be
                    // destroyed when 'd' is at the end of this function, so we
                    // "steal" its dict_ to avoid an unnecessary copy. We know
                    // the refcount_ is one, so nobody else is using the Mul
                    // except us.

Possible workaround: use /* */ instead of //, then clang-format seems to let it be. We can do such changes once this PR is merged.

certik · 2016-03-30T16:59:33Z

Besides the column limit, I don't have any other suggestions. The clang-format seems to do a very good job and I agree with all the changes that it made.

certik · 2016-03-30T17:14:17Z

Another issue that I found is that when I install clang-format from source (hashdist/hashstack#941), it creates a clang-format binary by default. So then the test_format_local.sh script fails, as it expects clang-format-3.7 on linux.

srajangarg · 2016-03-30T21:05:43Z

@certik Don't you feel 80 characters is a bit too less? I know it is the defacto standard, but still. Also what about lines which are just longer than 80 characters?

What do you suggest about the clang-format executable issue? I feel it's very minor, and we can ask developers to install it via their respective package managers.

certik · 2016-03-31T14:23:35Z

I don't think 80 is too narrow. Here is the patch: srajangarg#2, the code looks great. I don't understand your question about a line longer than 80 characters. I don't think there is any. If you want some pointers why 80 is a good choice (in the past and today), see e.g.:

https://www.emacswiki.org/emacs/EightyColumnRule
https://www.reddit.com/r/programming/comments/2nkntp/does_column_width_80_make_sense_in_2014/

It really is a great choice.

As to clang-format, all we need to do is make the script robust so that it works in most cases. Most likely using the following algorithm:

if clang-format-3.7 exists in the path, use it
if clang-format exists, check that it is the 3.7 version using:

$ clang-format -version
clang-format version 3.7.1 (tags/RELEASE_371/final)

and if so, use it.

Otherwise fail. (We can possibly even skip the version detection in step 2.)

Use 80 characters column limit

srajangarg · 2016-03-31T20:23:54Z

@certik, considering all the inputs on this, should I go ahead with a new PR?

certik · 2016-03-31T20:43:34Z

@srajangarg looks like you addressed all the issues raised in the PR? If so, I would go ahead and create a nice, clean new PR off top of the latest master as I suggested above.

srajangarg added 4 commits March 23, 2016 14:50

added .clang-format, and formatted entire master

e42ff12

made changes for travis for clang-format

9e1d4c7

some residual file formatting changes and added local script for form…

7a909ca

…atting

travis_clang should run only on linux in travis

8a00dcb

Sumith1896 reviewed Mar 26, 2016
View reviewed changes

Sumith1896 mentioned this pull request Mar 26, 2016

Remove whitespace #888

Closed

nishnik reviewed Mar 29, 2016
View reviewed changes

.clang-format

Standard: Cpp11

TabWidth: 4

UseTab: Never

Copy link

Contributor

nishnik Mar 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not needed.

srajangarg added 2 commits March 30, 2016 01:22

changed template style, new line fixes and also some comments changed

4edc920

merged with master, removed conflicts

7ba80e8

isuruf reviewed Mar 30, 2016
View reviewed changes

increased col limit, removed spaces before public/private, removed al…

12adac9

…l single line functions except empty functions'

certik reviewed Mar 30, 2016
View reviewed changes

certik added 2 commits March 31, 2016 08:20

Use 80 characters column limit

a62068d

Reformat the code

418b5e0

Merge pull request #2 from certik/pr-882-column_fix

d52e589

Use 80 characters column limit

srajangarg mentioned this pull request Mar 31, 2016

Clang format final #898

Merged

srajangarg closed this Mar 31, 2016

srajangarg deleted the clang-format branch June 6, 2016 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang-format #882

clang-format #882

srajangarg commented Mar 23, 2016

srajangarg commented Mar 23, 2016

Sumith1896 commented Mar 23, 2016

Sumith1896 commented Mar 26, 2016

Sumith1896 Mar 26, 2016

Sumith1896 Mar 26, 2016

srajangarg Mar 27, 2016

Sumith1896 Mar 27, 2016

srajangarg Mar 27, 2016

Sumith1896 commented Mar 26, 2016

srajangarg commented Mar 27, 2016

srajangarg commented Mar 27, 2016

nishnik Mar 29, 2016

srajangarg commented Mar 29, 2016

Sumith1896 commented Mar 29, 2016

isuruf Mar 30, 2016

srajangarg Mar 30, 2016

isuruf Mar 30, 2016

srajangarg Mar 30, 2016

isuruf Mar 30, 2016

certik commented Mar 30, 2016

certik commented Mar 30, 2016

certik Mar 30, 2016

certik commented Mar 30, 2016

certik commented Mar 30, 2016

certik commented Mar 30, 2016

certik commented Mar 30, 2016

srajangarg commented Mar 30, 2016

certik commented Mar 31, 2016

srajangarg commented Mar 31, 2016

certik commented Mar 31, 2016

clang-format #882

clang-format #882

Conversation

srajangarg commented Mar 23, 2016

srajangarg commented Mar 23, 2016

Sumith1896 commented Mar 23, 2016

Sumith1896 commented Mar 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sumith1896 commented Mar 26, 2016

srajangarg commented Mar 27, 2016

srajangarg commented Mar 27, 2016

Choose a reason for hiding this comment

srajangarg commented Mar 29, 2016

Sumith1896 commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Mar 30, 2016

certik commented Mar 30, 2016

Choose a reason for hiding this comment

certik commented Mar 30, 2016

certik commented Mar 30, 2016

certik commented Mar 30, 2016

certik commented Mar 30, 2016

srajangarg commented Mar 30, 2016

certik commented Mar 31, 2016

srajangarg commented Mar 31, 2016

certik commented Mar 31, 2016