Edward Hart <edward.dan.hart@gmail.com> writes:

> Hi Joakim,

Hi!

> The Imenu functionality is excellent and I can't wait to get it committed.

Great :)

> I've only found one bug in the patch which involves function
> definitions and program definitions in the same file. If I create an
> index for such a file, the index entries for the first
> program/function are not listed under the program/function's name but
> just "File section", "Working-storage section", etc. The entries for
> the following programs/functions are listed under their names, as
> expected.

That was actually intentional, or well, semi intentional anyway ;) I was
debating (with myself) what would look best, I had some idea that the
first program/function definition in the file would be the "main" one
and that that should get the "main" menu items. But I will change it
so that all subprograms are under their respective names. Maybe also put
the subprogram submenu first?

> I have a few suggestions for the code itself:
>
> * By convention, COBOL keywords are written in uppercase and I think
> the regexps should be changed to reflect that.

Will do!

> * The code assumes all programs have IDENTIFICATION DIVISION headers, however the header is optional. Match against PROGRAM/FUNCTION-ID instead using cobol--function-id-name-re instead (which is
>  equivalent to cobol--imenu-program-name).

Huh, I had no idea that header was optional :) I will change it to
cobol--function-id-name-re (I actually meant to use the already defined
regexps all along, but I got slightly lost in the maze of all regexps :))

> * The "^[ \t]*\\([[:digit:]]\\{1,2\\}\\|[fsr]d\\)[ \t]+\\(\\w+\\)" regexp can be replaced with cobol--generic-declaration-re.

Likewise.

> Two very useful features I'd like to suggest would be peeking at
> (displaying the line a data item is defined on in a temporary buffer
> would be good enough) and jumping to data definitions. Adding key
> bindings for them would be an added bonus.

I don't really understand what you mean here. Isn't jumping to
definitions what Imenu does? :) Or do you mean something more xref-like?

Does xref have a generic imenu backend? Should it?

Actually I think that peeking should be a feature of xref (xref-peek?).
It sounds a bit like xref-show-location-at-point, but maybe not really?

Anyway, I think that these things are maybe better left to a
xref-backend for cobol-mode (which I would love to write :)). In general
I'm quite excited at the prospect of having uniform keybindings and
interfaces across modes for things like go to definition (and peek at
definition).

Come to think of it, ctags already support COBOL, so you should be able
to get xref functionality via that, I think.

> Regards,
> Edward

Thanks for the feedback! I will try to have a new version ready before
the end of the week.

-- Joakim