[hawkmoth] Re: [PATCH v2 12/15] hawkmoth: fix documentation of anonymous types

  • From: Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx>
  • To: Jani Nikula <jani@xxxxxxxxxx>
  • Date: Wed, 30 Jan 2019 22:43:11 +0100

Hey,

Reviving this stale thread. Sorry in advance for the lengthy email.

On 23:06:51 2019-01-24, Jani Nikula wrote:

On Wed, 23 Jan 2019, Jani Nikula <jani@xxxxxxxxxx> wrote:
On Wed, 23 Jan 2019, Bruno Santos <brunomanuelsantos@xxxxxxxxxxxxxxxxxx> 
wrote:
On 20:37:19 2019-01-23, Jani Nikula wrote:
FWIW, in general, this is my rough list of priorities:

- Handle bog standard most common use cases.

- Implementation simplicity. If there's an edge case that requires a
  fair amount of code or complexity, ignore it. In particular, if
  there's a reasonable workaround.

- Principle of least surprise. What would the user mean to document?

Agree up to here...


- Doxygen/Javadoc/etc. compatibility.

This I think it's more of a /nice to have/, the same way we're not
putting particular emphasis on supporting additional syntaxes and all
that it entails.

With that said, this patch's behaviour is different from Doxygen, but
only in that Doxygen output is broken in the highlighted case.

We're in agreement here I think. Definitely no focus on any additional
or alternative syntaxes. More like, given more than one way to do
something, and all else being equal, follow what others did. And if
others do something stupid, opt for the sane choice instead.

I'd like Hawkmoth to offer a simple and straighforward alternative to
e.g. Doxygen+Breathe, and when simple is not enough, I won't hesitate to
recommend people to go for the more complicated alternatives. But the
basics need to be familiar enough.

For example, having both the tokenizer and AST passes is extra
complexity, but it's required to properly handle /** style comments and
to document macros.

As to this specific patch, there's something that feels a bit too quirky
now. I think one of the things you've overlooked is
cursor.is_anonymous(), which will make things quite a bit nicer. I'll
try to look at other things that we could get directly from clang,
particularly for avoiding the split/join on the cursor.type.spelling.

It's possible, but I tried cursor.is_anonymous before settling on an
ugly string search. I didn't follow it through to the C code, but I
suspect this means something quite different or at least it's broken for
me.

Clang does give the type a name, but it's a long string including the
path, filename and line of the declaration with the words 'anonymous
structure' or something. Not something I'd like to see in the
documentation to be honest, hence the replacement for '<anonymous>'.

FYI, I just retested it, and I can't get is_anonymous to do what I would
expect it to. Maybe an issue with my build only?

I'll need to play with this when I have the time. Now that the parser is
in a better shape, it's actually possible to do so!

Okay, it doesn't work.

However, cursor.spelling is always None for unnamed structs, unions and
enums.

Actually it's '', not None.

Now, this would almost work *except* that for standard types, cursor
spelling is also '', but the type is most definitely not anonymous. I.e.
this now satisfies the definition of anonymous:

        /** doc */
        int var;

I managed to use cursor.spelling in only one of the 3 instances in the
original patch. We would still need it in one and uglier code to use it
in the other one.

All in all, I'd rather use the string search everywhere.

It's also better because the test is not done in the type cursor, but
the instance cursor. This leads to nicer code overall.

I can however abstract the string search, making it nicer to look at.


I was able to make the simple case of anonymous enums work the way I
want. Patch below, against current master. However, that doesn't handle
variable definitions, and it breaks typedefs.

I originally agreed with your suggestion as to how it should work, but I
now believe it's the wrong way to go entirely.

If we follow this new approach, then:

        /** Is this the enumerator or the instance? */
        enum {...} instance;

My 1st idea was that the comment should be assigned to the type 1st and
if that was anonymous, a simple text string, but never the instance.
That's also the case in your patch.

However, to allow the documentation of the instance, we would have at
least to recommend that users don't use the pattern above at all, and I
don't think that's a valid recommendation because there's no other way
to instance a variable with that type.

Note that the way it worked in the original proposal, carries with it a
similar recommendation, but one that is reasonable I think:

        /** 
         * This will document the type; separate the instance to
         * document that as well. It's possible to do so because the
         * type is named.
         */
         enum type {...} instance;

I.e. the comment in the 1st snippet has to belong to the instance if we
want to allow the user to document the instance at all.

So I arrive at a different set of rules: instance, then type, then text.
All the functionality of the original patch, but the possibility to have
uninstantiated and anonymous enumerators as well. Still an improvement,
so why not?

1. We can easily find the type of an instance when we hold that
   instance's cursor (necessary to attribute the documentation to the
   instance).
   
   But to do this we also need to check if there is an instance while
   parsing the type if the type is anonymous. If there is an instance we
   do nothing because the instance will eventually pick up the
   documentation by finding its way back. If there isn't an instance we
   need to issue the documentation as a text block.
   
   But how can we find the instance from the type cursor? I believe this
   would require a 2nd pass over the cursors building our own AST.
   That's far from trivial.

2. This is a superset of the originally proposed solution. I.e. the
   proposed solution would be a step in the desired direction either
   way. This could come later as a further improvement. This point I had
   made earlier actually.

3. Not doing this at all would simply require that the user names the
   enumerator if he thinks it's worth documenting. I hardly think this
   is unreasonable in any way and I think it's totally worth it if this
   means we dodge that hairy issue 1.


Number 1. is the breaking point, so if you have a suggestion to solve
this (or I totally missed some detail of Clang's AST), then we may still
do it.

What do you think?


And the typedefs are another can of worms, both for enums and
structs/unions, and somewhat broken in current master already.

Typedefs of anonymous structs work as expected (without the patch at the
end):

      /** documents type_t */
      typedef struct { ... } type_t;

I can confirm this works with the original solution.


However named things get complicated:

      /** documents struct named, not type_t */
      typedef struct named { ... } type_t;

The typedef-struct.c test case even tests this, and I'm not sure I like
it. It is true the named one creates two usable types, 'struct named'
and 'type_t'.

I played with the code, and it seems to me fixing this would get pretty
complicated pretty fast. The comment gets attached to the struct, not
the typedef, and you can't get to the typedef except by traversing the
cursors at the top level.

Is it unreasonable to recommend either:

      /** document type_t */
      typedef struct { ... } type_t;

or:

      /** document struct named */
      struct named { ... };

      /** document type_t */
        typedef struct named type_t;

and discourage the combination of the latter?

Seems like the same thing when combining structs/unions/variables with
instances.

+1

I assume no action here.

Cheers,
Bruno


BR,
Jani.

Other related posts:

  1. Home
  2. hawkmoth
  3. Archive
  4. 01-2019