Octave coding style questions

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Octave coding style questions

Rik-4
All,

I recently style-checked the Octave code base using a combination of astyle
set for GNU coding standards and some Perl post-processing.  It still
requires too much human inspection to review what the scripts find.  One of
the recurring issues that gets flagged is the indentation of enum blocks. 
The current code uses double indentation as is done for flow-control blocks
like those following if, for, while, etc.

enum color
  {
    RED,
    GREEN,
    BLUE
  };

astyle would like to remove the extra indent and make this

enum color
{
  RED,
  GREEN,
  BLUE
};

I don't think it particularly matters, and would prefer to let astyle
correct this and then stop harassing me.  Are there any useful arguments
agains this change?

On a related note, how should class definitions being handled?  In some
cases we use the C++ keyword followed immediately by the class name, and in
other cases "class" appears by itself followed by the name of the class on
it's own line.  This is vaguely similar to the way we define functions
where the return value appears first on its own line followed by the name
of the function.  Examples:

Style #1
class callback_props
{

Style #2
class
callback_event
{

A second decision is whether parent classes  should appear on the line with
the name of the class, or also on their own

Style #1
class
callback_event : public base_graphics_event
{

Style #2
class
callback_event
  : public base_graphics_event
{

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Octave coding style questions

Mike Miller-4
On Sun, Jan 12, 2020 at 10:38:26 -0800, Rik wrote:

> astyle would like to remove the extra indent and make this
>
> enum color
> {
>   RED,
>   GREEN,
>   BLUE
> };
>
> I don't think it particularly matters, and would prefer to let astyle
> correct this and then stop harassing me.  Are there any useful arguments
> agains this change?
No, and this change is consistent with the GNU standards
(https://www.gnu.org/prep/standards/html_node/Formatting.html#index-enum-types_002c-formatting)

> On a related note, how should class definitions being handled?  In some
> cases we use the C++ keyword followed immediately by the class name, and in
> other cases "class" appears by itself followed by the name of the class on
> it's own line.  This is vaguely similar to the way we define functions
> where the return value appears first on its own line followed by the name
> of the function.  Examples:
>
> Style #1
> class callback_props
> {
>
> Style #2
> class
> callback_event
> {
I think the first choice is more sensible here. The GNU C style does not
recommend splitting after the keyword "enum" or "struct" for example,
and "class" is in the same family as "enum" and "struct" in C++.

> A second decision is whether parent classes  should appear on the line with
> the name of the class, or also on their own
>
> Style #1
> class
> callback_event : public base_graphics_event
> {
>
> Style #2
> class
> callback_event
>   : public base_graphics_event
> {
I think the first choice is more sensible here, with the "class" keyword
also on the same line

    class callback_event : public base_graphics_event
    {

When the class names are long enough to warrant folding, then colon
makes sense as a fold point

    class exceptionally_long_derived_class_name
      : public exceptionally_long_base_class_name
    {

Cheers,

--
mike

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Octave coding style questions

John W. Eaton
Administrator
On 1/12/20 4:02 PM, Mike Miller wrote:

> On Sun, Jan 12, 2020 at 10:38:26 -0800, Rik wrote:
>> astyle would like to remove the extra indent and make this
>>
>> enum color
>> {
>>    RED,
>>    GREEN,
>>    BLUE
>> };
>>
>> I don't think it particularly matters, and would prefer to let astyle
>> correct this and then stop harassing me.  Are there any useful arguments
>> agains this change?
>
> No, and this change is consistent with the GNU standards
> (https://www.gnu.org/prep/standards/html_node/Formatting.html#index-enum-types_002c-formatting)

This change is fine with me, though Emacs CC mode with the gnu style
appears to indent the enum block by default but it shouldn't be too hard
to change that.

>> On a related note, how should class definitions being handled?  In some
>> cases we use the C++ keyword followed immediately by the class name, and in
>> other cases "class" appears by itself followed by the name of the class on
>> it's own line.  This is vaguely similar to the way we define functions
>> where the return value appears first on its own line followed by the name
>> of the function.  Examples:
>>
>> Style #1
>> class callback_props
>> {
>>
>> Style #2
>> class
>> callback_event
>> {
>
> I think the first choice is more sensible here. The GNU C style does not
> recommend splitting after the keyword "enum" or "struct" for example,
> and "class" is in the same family as "enum" and "struct" in C++.

Yes, style #1 seems better to me as well.

>> A second decision is whether parent classes  should appear on the line with
>> the name of the class, or also on their own
>>
>> Style #1
>> class
>> callback_event : public base_graphics_event
>> {
>>
>> Style #2
>> class
>> callback_event
>>    : public base_graphics_event
>> {
>
> I think the first choice is more sensible here, with the "class" keyword
> also on the same line
>
>      class callback_event : public base_graphics_event
>      {
>
> When the class names are long enough to warrant folding, then colon
> makes sense as a fold point
>
>      class exceptionally_long_derived_class_name
>        : public exceptionally_long_base_class_name
>      {

I agree.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Octave coding style questions

siko1056
In reply to this post by Rik-4
On 1/13/20 3:38 AM, Rik wrote:
>
> I recently style-checked the Octave code base using a combination of astyle
> set for GNU coding standards and some Perl post-processing.  It still
> requires too much human inspection to review what the scripts find.

Rik, thanks for doing all this work.  I think ideally, there should be
no/not much human interaction at all.

Was it possible to document your exact Astyle command in the wiki [1]
and maybe upload your Perl scripts to "build-aux"?  Even better a short
script like "build-aux/enforce_Octave_cpp_style.sh" calling astyle and
your perl scripts correctly?

This might save many whitespace change "ping-pongs" before releases, if
all maintainers can enforce the same style guide "mechanically" and not
by interpreting [1] subjectively.  Especially, some patches became
inapplicable after some time this way.

> I don't think it particularly matters, and would prefer to let astyle
> correct this and then stop harassing me.  Are there any useful arguments
> agains this change?

I think astyle is a solid tool for this work and I have no objections to
make maintenance easier.


Just tried to let astyle help me using the `--style=gnu
--indent=spaces=2` options [2]:

```sh
cp -R default default2 && cd default2 && rm -Rf .hg gnulib libgnu
find . -name "*.h" -or -name "*.c" -or -name "*.cc" | xargs astyle
--style=gnu --indent=spaces=2 > astyle.txt
cat astyle.txt                    | wc -l  #1253
cat astyle.txt | grep "Formatted" | wc -l  #881
```

In 881 from 1253 files there was formatting necessary.  Thus I cannot
reproduce your style automatically.

Kai


[1] https://wiki.octave.org/C%2B%2B_style_guide
[2] http://astyle.sourceforge.net/astyle.html#_style=gnu

Reply | Threaded
Open this post in threaded view
|

Re: astyle configuration

Rik-4
On 01/13/2020 01:48 AM, Kai Torben Ohlhus wrote:

> On 1/13/20 3:38 AM, Rik wrote:
>> I recently style-checked the Octave code base using a combination of astyle
>> set for GNU coding standards and some Perl post-processing.  It still
>> requires too much human inspection to review what the scripts find.
> Rik, thanks for doing all this work.  I think ideally, there should be
> no/not much human interaction at all.
>
> Was it possible to document your exact Astyle command in the wiki [1]
> and maybe upload your Perl scripts to "build-aux"?  Even better a short
> script like "build-aux/enforce_Octave_cpp_style.sh" calling astyle and
> your perl scripts correctly?
>
> This might save many whitespace change "ping-pongs" before releases, if
> all maintainers can enforce the same style guide "mechanically" and not
> by interpreting [1] subjectively.  Especially, some patches became
> inapplicable after some time this way.
>
>> I don't think it particularly matters, and would prefer to let astyle
>> correct this and then stop harassing me.  Are there any useful arguments
>> agains this change?
> I think astyle is a solid tool for this work and I have no objections to
> make maintenance easier.
>
>
> Just tried to let astyle help me using the `--style=gnu
> --indent=spaces=2` options [2]:
>
> ```sh
> cp -R default default2 && cd default2 && rm -Rf .hg gnulib libgnu
> find . -name "*.h" -or -name "*.c" -or -name "*.cc" | xargs astyle
> --style=gnu --indent=spaces=2 > astyle.txt
> cat astyle.txt                    | wc -l  #1253
> cat astyle.txt | grep "Formatted" | wc -l  #881
> ```
>
> In 881 from 1253 files there was formatting necessary.  Thus I cannot
> reproduce your style automatically.
>
> Kai
>
>
> [1] https://wiki.octave.org/C%2B%2B_style_guide
> [2] http://astyle.sourceforge.net/astyle.html#_style=gnu
>
I will work a bit more on this.  I use a separate file called astylerc
which throws many more configuration switches

--style=gnu
--indent=spaces=2
--max-code-length=80
--min-conditional-indent=0
--indent-namespaces
--indent-labels
--indent-cases
--pad-header
--keep-one-line-blocks
--keep-one-line-statements

--Rik


Reply | Threaded
Open this post in threaded view
|

Re: Octave coding style questions

Rik-4
In reply to this post by Rik-4
On 01/12/2020 08:40 PM, [hidden email] wrote:
Subject:
Re: Octave coding style questions
From:
"John W. Eaton" [hidden email]
Date:
01/12/2020 08:40 PM
To:
Mike Miller [hidden email], [hidden email]
List-Post:
[hidden email]
Content-Transfer-Encoding:
quoted-printable
Precedence:
list
MIME-Version:
1.0
References:
<MTAwMDAzMy5ub21hZA.1578854312@quikprotect> [hidden email]
In-Reply-To:
[hidden email]
Message-ID:
[hidden email]
Content-Type:
text/plain; charset=utf-8; format=flowed
Message:
6

On 1/12/20 4:02 PM, Mike Miller wrote:
On Sun, Jan 12, 2020 at 10:38:26 -0800, Rik wrote:
astyle would like to remove the extra indent and make this

enum color
{
   RED,
   GREEN,
   BLUE
};

I don't think it particularly matters, and would prefer to let astyle
correct this and then stop harassing me.  Are there any useful arguments
agains this change?

No, and this change is consistent with the GNU standards
(https://www.gnu.org/prep/standards/html_node/Formatting.html#index-enum-types_002c-formatting)

This change is fine with me, though Emacs CC mode with the gnu style appears to indent the enum block by default but it shouldn't be too hard to change that.

On a related note, how should class definitions being handled?  In some
cases we use the C++ keyword followed immediately by the class name, and in
other cases "class" appears by itself followed by the name of the class on
it's own line.  This is vaguely similar to the way we define functions
where the return value appears first on its own line followed by the name
of the function.  Examples:

Style #1
class callback_props
{

Style #2
class
callback_event
{

I think the first choice is more sensible here. The GNU C style does not
recommend splitting after the keyword "enum" or "struct" for example,
and "class" is in the same family as "enum" and "struct" in C++.

Yes, style #1 seems better to me as well.

A second decision is whether parent classes  should appear on the line with
the name of the class, or also on their own

Style #1
class
callback_event : public base_graphics_event
{

Style #2
class
callback_event
   : public base_graphics_event
{

I think the first choice is more sensible here, with the "class" keyword
also on the same line

     class callback_event : public base_graphics_event
     {

When the class names are long enough to warrant folding, then colon
makes sense as a fold point

     class exceptionally_long_derived_class_name
       : public exceptionally_long_base_class_name
     {

I agree.

jwe

Good.  This was the way I was leaning on each question as well.

--Rik
Reply | Threaded
Open this post in threaded view
|

Re: astyle configuration

Gene Harvey
In reply to this post by Rik-4
On Mon, Jan 13, 2020 at 9:44 AM Rik <[hidden email]> wrote:

> I will work a bit more on this.  I use a separate file called astylerc
> which throws many more configuration switches
>
> --style=gnu
> --indent=spaces=2
> --max-code-length=80
> --min-conditional-indent=0
> --indent-namespaces
> --indent-labels
> --indent-cases
> --pad-header
> --keep-one-line-blocks
> --keep-one-line-statements
>
> --Rik

Would it be a good idea to use clang-format instead? It has a more
configuration
options and it's more robust for handling edge cases (it uses clang's lexer).

Unfortunately, I haven't been able to emulate Octave's style perfectly
with it. The main issue
for me is that it doesn't allow for separate syntax with regards to *
and & placement other
than by using an option which infers the style based on the current
placement of those tokens.
However, other than that I've come decently close to emulating the style.

Gene

Reply | Threaded
Open this post in threaded view
|

Re: astyle configuration

Rik-4
On 01/13/2020 01:37 PM, Gene Harvey wrote:

> On Mon, Jan 13, 2020 at 9:44 AM Rik <[hidden email]> wrote:
>> I will work a bit more on this.  I use a separate file called astylerc
>> which throws many more configuration switches
>>
>> --style=gnu
>> --indent=spaces=2
>> --max-code-length=80
>> --min-conditional-indent=0
>> --indent-namespaces
>> --indent-labels
>> --indent-cases
>> --pad-header
>> --keep-one-line-blocks
>> --keep-one-line-statements
>>
>> --Rik
> Would it be a good idea to use clang-format instead? It has a more
> configuration
> options and it's more robust for handling edge cases (it uses clang's lexer).
>
> Unfortunately, I haven't been able to emulate Octave's style perfectly
> with it. The main issue
> for me is that it doesn't allow for separate syntax with regards to *
> and & placement other
> than by using an option which infers the style based on the current
> placement of those tokens.
> However, other than that I've come decently close to emulating the style.
I now have pretty good conformance with Octave's existing code base.  The
only reason, for me, to incur the switching costs would be if clang-format
handled the alignment of tertiary operators in a better manner.  An example is

octave_value retval = (! some_variable.isempty ()) ? true
                                                   : false;

astyle wants to pull the second line beginning with a colon back towards
column 1 rather than leaving it aligned under the '?' character.

My current astylerc is

--style=gnu
--indent=spaces=2
--max-code-length=80
--min-conditional-indent=1
--indent-cases
--indent-labels
--indent-namespaces
##--indent-preproc-block
##--indent-preproc-cond
##--indent-preproc-define
--pad-header
--pad-comma
--align-pointer=name
--align-reference=type
--attach-closing-while
--close-templates              # now that C++11 is required
--keep-one-line-blocks
--keep-one-line-statements

--Rik



Reply | Threaded
Open this post in threaded view
|

Re: astyle configuration

siko1056
On 1/14/20 7:28 AM, Rik wrote:

> On 01/13/2020 01:37 PM, Gene Harvey wrote:
>>
>> Would it be a good idea to use clang-format instead? It has a more
>> configuration options and it's more robust for handling edge cases
>> (it uses clang's lexer).
>>
>> Unfortunately, I haven't been able to emulate Octave's style
>> perfectly with it.
>>
>> [...]
>>
>> However, other than that I've come decently close to emulating the
>> style.


@Gene: can you post your clang-format options as well for comparing the
results easier?


On 1/14/20 7:28 AM, Rik wrote:

> My current astylerc is
>
> --style=gnu
> --indent=spaces=2
> --max-code-length=80
> --min-conditional-indent=1
> --indent-cases
> --indent-labels
> --indent-namespaces
> ##--indent-preproc-block
> ##--indent-preproc-cond
> ##--indent-preproc-define
> --pad-header
> --pad-comma
> --align-pointer=name
> --align-reference=type
> --attach-closing-while
> --close-templates              # now that C++11 is required
> --keep-one-line-blocks
> --keep-one-line-statements
>

@Rik with your current astyle settings, I still get 758 of 1253
formattings and started using a script adding "--suffix=none" to avoid
astyle to create backups

```sh
#!/bin/sh

ASTYLE_OPTS=" \
  --suffix=none \
  --style=gnu \
  --indent=spaces=2 \
  --max-code-length=80 \
  --min-conditional-indent=1 \
  --indent-cases \
  --indent-labels \
  --indent-namespaces \
  --pad-header \
  --pad-comma \
  --align-pointer=name \
  --align-reference=type \
  --attach-closing-while \
  --close-templates \
  --keep-one-line-blocks \
  --keep-one-line-statements"

find . \
  -type d \( -path ./.hg -o -path ./gnulib -o -path ./libgnu \) -prune
-false \
  -o -type f -name "*.h" -or -name "*.c" -or -name "*.cc" \
  | xargs astyle ${ASTYLE_OPTS} > astyle.txt

cat astyle.txt | wc -l
cat astyle.txt | grep "Formatted" | wc -l
```

to call directly inside the Octave development repository, excluding
some irrelevant directories.  To undo the mess, run

  hg revert --no-backup --all



On 1/14/20 7:28 AM, Rik wrote:

> The only reason, for me, to incur the switching costs would be
> if clang-format handled the alignment of tertiary operators in
> a better manner.  An example is
>
> octave_value retval = (! some_variable.isempty ()) ? true
>                                                    : false;
>
> astyle wants to pull the second line beginning with a colon back
> towards column 1 rather than leaving it aligned under the '?'
> character.
>

I think we should in general change to this layout in such cases:

  octave_value retval = (! some_variable.isempty ())
                        ? true
                        : false;

Then astyle also does not break anything with your configuration and for
me it is even more clearer, where to look for the condition?true:false
parts of that operator.

clang-format offers BreakBeforeTernaryOperators [1] but first want to
have a more complete configuration by Gene before experimenting too much.

Kai


[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html







Reply | Threaded
Open this post in threaded view
|

Re: astyle configuration

John W. Eaton
Administrator
On 1/13/20 11:15 PM, Kai Torben Ohlhus wrote:

> I think we should in general change to this layout in such cases:
>
>    octave_value retval = (! some_variable.isempty ())
>                          ? true
>                          : false;
>
> Then astyle also does not break anything with your configuration and for
> me it is even more clearer, where to look for the condition?true:false
> parts of that operator.

If you write it like this

   octave_value retval = (! some_variable.is_empty ()
                          ? do_something (with, some, arguments)
                          : do_something_else (with, other, arguments));

then Emacs CC mode will do the right thing with indenting the ?: to line
up inside the outer parens.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: astyle configuration

Gene Harvey
I've attached the configuration I was using.

Observations based on using it on a random file:
- It's worse than I remember.
- Almost every file would have a diff because the space after the
`defined` macro would be removed.
- DerivePointerAlignment applies either left or right-alignment to
both * and &, rather than independently.
- Matrix indexing function calls, where there isn't a space before the
parenthesis, would become uniform with the usual function calls.
- Binary operators would have spaces surrounding them.
- clang-format prefers to place = on a new line before doing any other wrapping.

So, basically the whole style would change. However, the obvious
benefit is that we save brain cells instead of trying to perfectly
format everything manually.

Gene

On Mon, Jan 13, 2020 at 11:26 PM John W. Eaton <[hidden email]> wrote:

>
> On 1/13/20 11:15 PM, Kai Torben Ohlhus wrote:
>
> > I think we should in general change to this layout in such cases:
> >
> >    octave_value retval = (! some_variable.isempty ())
> >                          ? true
> >                          : false;
> >
> > Then astyle also does not break anything with your configuration and for
> > me it is even more clearer, where to look for the condition?true:false
> > parts of that operator.
>
> If you write it like this
>
>    octave_value retval = (! some_variable.is_empty ()
>                           ? do_something (with, some, arguments)
>                           : do_something_else (with, other, arguments));
>
> then Emacs CC mode will do the right thing with indenting the ?: to line
> up inside the outer parens.
>
> jwe
>

.clang-format (4K) Download Attachment