[Emacs-ada-mode] Indentation of package aspects

[Piotr Trojanek]

Hi,

with the latest ada-mode 5.1.9 I get a runtime error when indenting
package aspects.

Test and patch attached. Note that I do not know if my decision about
how to indent the package aspect is in line with the style promoted by
the ada-mode.

[Simon Wright]

Post by Piotr Trojanek
with the latest ada-mode 5.1.9 I get a runtime error when indenting
package aspects.
Test and patch attached. Note that I do not know if my decision about
how to indent the package aspect is in line with the style promoted by
the ada-mode.

I think the patch should be at line 687 (i.e. in alphabetical order), and, comparing with the indentation for private_extension_declaration, private_type_declaration, the indentation should be to the current level:

(package_specification
;; package P
;; with
;; indenting 'with'
(current-indentation))

[Piotr Trojanek]

I agree. Yet with alphabetical order the branch for
abstract_subprogram_declaration should be the first one, not the last
one, right?

[Simon Wright]

Post by Piotr Trojanek
I agree. Yet with alphabetical order the branch for
abstract_subprogram_declaration should be the first one, not the last
one, right?

What happened there was that that list of non-terminals originally started with subprogram_body, which was in alphabetical order; Stephe added abstract_subprogram_declaration & expression_function_declaration after the fact. I suppose we could move the entire group to the start of this case statement (line 554). See also full_type_declaration etc.!

[Piotr Trojanek]

Post by Simon Wright
What happened there was that that list of non-terminals originally started with subprogram_body, which was in alphabetical order; Stephe added abstract_subprogram_declaration & expression_function_declaration after the fact. I suppose we could move the entire group to the start of this case statement (line 554). See also full_type_declaration etc.!

OK, thank you for clarifying. I just looked at the last group, which
was not in order, and decided not to guess what were your intentions
about sorting.

Anyway, here is a better patch. It is sorted and fixes problem with
aspect indentation both for package specs and bodies.

[Stephen Leake]

Post by Simon Wright

Post by Piotr Trojanek

Post by Simon Wright
I think the patch should be at line 687 (i.e. in alphabetical
order), and, comparing with the indentation for
private_extension_declaration, private_type_declaration, the

I agree. Yet with alphabetical order the branch for
abstract_subprogram_declaration should be the first one, not the last
one, right?

What happened there was that that list of non-terminals originally
started with subprogram_body, which was in alphabetical order; Stephe
added abstract_subprogram_declaration &
expression_function_declaration after the fact.

right; prefering to not duplicate code. We could pull out the case body
as a subprogram and split the cases.

Post by Simon Wright
I suppose we could
move the entire group to the start of this case statement (line 554).

I kept this at the place "subprogram_body" and "subprogram_declaration"
belongs, because the others are variations on that.

perhaps:

((subprogram_body
subprogram_declaration
subprogram_specification

;; shared code, but out of alphabetical order:
abstract_subprogram_declaration
expression_function_declaration
null_procedure_declaration)
(cl-ecase (wisi-cache-token cache)
(IS

there should also be a cross-reference for the out of order ones, at the
place where they would be in order

Post by Simon Wright
See also full_type_declaration etc.!

yes; same fix.

[Stephen Leake]

Post by Piotr Trojanek
Hi,
with the latest ada-mode 5.1.9 I get a runtime error when indenting
package aspects.
Test and patch attached.

Thanks!

+ (package_specification
+ ;; package P

The first line of this comment should be a shortcut to the file
containing the test code: test/p.ads. That way, you can use a binding to
`find-file-at-point' (C-F12 for me) to navigate to directly to the test.

I'd prefer a longer name for the test file, or include this test in an
existing package. "SPARK_Mode" is recognized by GNAT GPL 2014 (which is
what I'm testing with), so this can go in an existing package;
ada_mode-nominal.ads. That changes the test of ada-*-statement-keyword.

+ ;; with SPARK_Mode => On
+ ;; indenting 'with'
+ (save-excursion
+ (wisi-backward-token)
+ (+ (current-column) ada-indent-broken)))

There's a bug in the grammar here. At this point in the code, point is
on the token containing the aspect, which is P. But it should be
"package", so the indentation is dependent on the keyword, not the name.
Then you would not need "wisi-backward-token". And it changes
"package_specification" to "package_declaration".

Post by Piotr Trojanek
Note that I do not know if my decision about
how to indent the package aspect is in line with the style promoted by
the ada-mode.

As Simon pointed out, aspects with types are aligned with "type", so
these should be aligned with "package". This looks better when there is
a list of aspects:

package Ada_Mode.Nominal
with
SPARK_Mode => On,
foo => bar
is

Full patch attached.

--
-- Stephe

[Piotr Trojanek]

On Mon, Feb 8, 2016 at 11:33 AM, Stephen Leake

Post by Stephen Leake
The first line of this comment should be a shortcut to the file
containing the test code: test/p.ads. That way, you can use a binding to
`find-file-at-point' (C-F12 for me) to navigate to directly to the test.

Right.

Post by Stephen Leake
I'd prefer a longer name for the test file, or include this test in an
existing package. "SPARK_Mode" is recognized by GNAT GPL 2014 (which is
what I'm testing with), so this can go in an existing package;
ada_mode-nominal.ads. That changes the test of ada-*-statement-keyword.

Right.

Post by Stephen Leake
+ ;; with SPARK_Mode => On
+ ;; indenting 'with'
+ (save-excursion
+ (wisi-backward-token)
+ (+ (current-column) ada-indent-broken)))
There's a bug in the grammar here. At this point in the code, point is
on the token containing the aspect, which is P. But it should be
"package", so the indentation is dependent on the keyword, not the name.
Then you would not need "wisi-backward-token". And it changes
"package_specification" to "package_declaration".

Containment in name seemed suspicious from the start, but I decided to
do the least intrusive fix.

Post by Stephen Leake
As Simon pointed out, aspects with types are aligned with "type", so
these should be aligned with "package". This looks better when there is
package Ada_Mode.Nominal
with
SPARK_Mode => On,
foo => bar
is

I fully agree with your justification.

Post by Stephen Leake
Full patch attached.

I will now extend this patch to other aspects that currently cause
indentation exceptions: in package body (which I already reported in
this thread), protected body and all the _stub constructs.

Thank you for review and all corrections!

[Stephen Leake]

Post by Piotr Trojanek
I will now extend this patch to other aspects that currently cause
indentation exceptions: in package body (which I already reported in
this thread), protected body and all the _stub constructs.

I just pushed a rev with the package body fix.

Post by Piotr Trojanek
Thank you for review and all corrections!

You're welcome.