Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autostruct/autounion/autoenum directives can't reference typedefs of anonymous structs/unions/enums #201

Closed
jnikula opened this issue Oct 21, 2023 · 9 comments

Comments

@jnikula
Copy link
Owner

jnikula commented Oct 21, 2023

The fixed up name is passed as decl_name to docstring, but the matching uses name, always failing to match.

@jnikula
Copy link
Owner Author

jnikula commented Oct 22, 2023

Presumably this only affects clang 15 and earlier, as clang 16 and later have the right name in cursor.spelling.

@BrunoMSantos
Copy link
Collaborator

BrunoMSantos commented Oct 22, 2023

Hmmm 🤔

Wouldn't the real reason be that we're documenting it as a struct and not a type? This kind of goes back to what I was saying in #190. I'm not sure what you're thinking of doing about it, but I don't like the idea of yet another workaround.

I'd rather go back to the original behaviour (consistent) and later think of allowing a shortcut and documenting the typedef (as a type) if the documentation applies do an anonymous struct/whatever that is defined inline with the typedef. That still sounds to me like the real solution.

Edit: Never mind. While I still stand by the above generally, this issue is not exactly about what I was thinking of when I read the title...

@jnikula
Copy link
Owner Author

jnikula commented Oct 22, 2023

Wouldn't the real reason be that we're documenting it as a struct and not a type?

Well, I don't think so. We've been documenting typedef struct { ... } name and typedef struct name { ... } foo as struct for the longest time. The only thing that changed with #190 is that the former now uses name for the struct name instead of @anonymous_<hash>.

Indeed, there's no bug with Clang 16+ because it gets the name from cursor.spelling, and passes that as the docstring name parameter. Clang 15 and earlier need the workaround to match Clang 16+ behaviour, but that name gets passed as decl_name to docstring. And the matching is done using name, not decl_name.

@jnikula
Copy link
Owner Author

jnikula commented Oct 22, 2023

I'm not sure yet if this is the right fix, but this does fix the issue:

diff --git a/src/hawkmoth/docstring.py b/src/hawkmoth/docstring.py
index f70ae01f948e..bc04aaa7c104 100644
--- a/src/hawkmoth/docstring.py
+++ b/src/hawkmoth/docstring.py
@@ -178,7 +178,7 @@ class Docstring():
         return self._decl_name if self._decl_name else self._name
 
     def get_name(self):
-        return self._name
+        return self._name if self._name else self._decl_name
 
     def get_line(self):
         return self._meta['line']

@BrunoMSantos
Copy link
Collaborator

Yup, I got there eventually ahah. Not that I think you should be able to reference such things with autostruct, it should be autotype, but that is indeed another matter.

@BrunoMSantos
Copy link
Collaborator

I'm not sure yet if this is the right fix, but this does fix the issue:

Hmm, it's not really obvious to me either. It's rather telling of how that part of the code still lacks in intuitiveness :/

@jnikula
Copy link
Owner Author

jnikula commented Oct 22, 2023

Yup, I got there eventually ahah. Not that I think you should be able to reference such things with autostruct, it should be autotype, but that is indeed another matter.

I maintain that it would be odd to document struct members for autotype, even if it works. And it might be seen as a bug in Sphinx and get fixed later.

If one wants to document the type separately, they should be defined separately:

struct name {
    ...
};

typedef struct name name_t;

@jnikula
Copy link
Owner Author

jnikula commented Oct 22, 2023

Hmm, it's not really obvious to me either. It's rather telling of how that part of the code still lacks in intuitiveness :/

I think this would be helped by passing the cursors (or abstractions of the cursors) to docstring.

@BrunoMSantos
Copy link
Collaborator

I think this would be helped by passing the cursors (or abstractions of the cursors) to docstring.

Indeed!

@jnikula jnikula mentioned this issue Nov 18, 2023
jnikula added a commit that referenced this issue Nov 18, 2023
Clang 15 and earlier have None for the cursor.spelling of typedefs of
anonymous structs/unions/enums. Clang 16 and later return the name of
the typedef. We address this in decl_name to get the Docstring right
across Clang versions, but the name filtering in Docstring._match() uses
name. This results in us being unable to reference typedefs of anonymous
entities with Clang 15 and earlier.

Fix it up by returning decl_name as name from Doccursor if
cursor.spelling is None.

In the long run, we should unify on just name. The decl_name was always
a workaround when we did the fixups in parser.py. With Doccursor, we
should be able to further clean that up, but for the time being, close
the known issue with the minimal fix.

It's probably theoretical, but in the case *both* name and decl_name
were None, we'd end up in infinite recursion if decl_name used self.name
instead of self._cc.spelling. It's a bit of a wart, but should also
cleaned up later.

Fixes #201
jnikula added a commit that referenced this issue Nov 18, 2023
Clang 15 and earlier have None for the cursor.spelling of typedefs of
anonymous structs/unions/enums. Clang 16 and later return the name of
the typedef. We address this in decl_name to get the Docstring right
across Clang versions, but the name filtering in Docstring._match() uses
name. This results in us being unable to reference typedefs of anonymous
entities with Clang 15 and earlier.

Fix it up by returning decl_name as name from Doccursor if
cursor.spelling is None.

In the long run, we should unify on just name. The decl_name was always
a workaround when we did the fixups in parser.py. With Doccursor, we
should be able to further clean that up, but for the time being, close
the known issue with the minimal fix.

It's probably theoretical, but in the case *both* name and decl_name
were None, we'd end up in infinite recursion if decl_name used self.name
instead of self._cc.spelling. It's a bit of a wart, but should also be
cleaned up later.

Fixes #201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants