RFR: 7903644: typedef of anonymous struct generates a redundant class
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct). This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted. I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests. ------------- Commit messages: - Add comment - Initial push Changes: https://git.openjdk.org/jextract/pull/195/files Webrev: https://webrevs.openjdk.org/?repo=jextract&pr=195&range=00 Issue: https://bugs.openjdk.org/browse/CODETOOLS-7903644 Stats: 35 lines in 5 files changed: 27 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jextract/pull/195.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/195/head:pull/195 PR: https://git.openjdk.org/jextract/pull/195
On Thu, 25 Jan 2024 17:24:45 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct).
This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted.
I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests.
src/main/java/org/openjdk/jextract/impl/NameMangler.java line 211:
209: // We may potentially generate a class for a typedef. Make sure 210: // class name is unique in the current nesting context. 211: javaName = curScope.uniqueNestedClassName(typedef.name());
This logic is a bit subtle: we have to avoid calling `uniqueNestedClassName` in case there's a struct that will inherit the typedef name - otherwise the mangler will see _two_ attempts to add the typedef name to the current scope, which will result in extra mangling. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/195#discussion_r1466707509
On Thu, 25 Jan 2024 17:25:59 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct).
This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted.
I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests.
src/main/java/org/openjdk/jextract/impl/NameMangler.java line 211:
209: // We may potentially generate a class for a typedef. Make sure 210: // class name is unique in the current nesting context. 211: javaName = curScope.uniqueNestedClassName(typedef.name());
This logic is a bit subtle: we have to avoid calling `uniqueNestedClassName` in case there's a struct that will inherit the typedef name - otherwise the mangler will see _two_ attempts to add the typedef name to the current scope, which will result in extra mangling.
This seems a bit ad-hoc... Suggestion: the `Scope` constructor tries to generate a unique nested class name if `parent` is not `null`. That's only the case when calling `newStruct`, which is only called from `visitScoped`. So, I think we can move the logic that computes the nested class name from the `Scope` constructor to `visitScoped` (to the `else` branch this patch adds). Then we can just pass the unmangled name from the `if` branch to `newStruct`, and there should be no need to have this logic here. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/195#discussion_r1466931107
On Thu, 25 Jan 2024 20:33:30 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
src/main/java/org/openjdk/jextract/impl/NameMangler.java line 211:
209: // We may potentially generate a class for a typedef. Make sure 210: // class name is unique in the current nesting context. 211: javaName = curScope.uniqueNestedClassName(typedef.name());
This logic is a bit subtle: we have to avoid calling `uniqueNestedClassName` in case there's a struct that will inherit the typedef name - otherwise the mangler will see _two_ attempts to add the typedef name to the current scope, which will result in extra mangling.
This seems a bit ad-hoc... Suggestion: the `Scope` constructor tries to generate a unique nested class name if `parent` is not `null`. That's only the case when calling `newStruct`, which is only called from `visitScoped`. So, I think we can move the logic that computes the nested class name from the `Scope` constructor to `visitScoped` (to the `else` branch this patch adds). Then we can just pass the unmangled name from the `if` branch to `newStruct`, and there should be no need to have this logic here.
Thanks for the suggestion - I did think of something similar, but had not realized we only needed the mangling in one place. Updated the code, looks a bit better I think. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/195#discussion_r1467006896
On Thu, 25 Jan 2024 17:24:45 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct).
This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted.
I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests.
Marked as reviewed by jvernee (Committer). ------------- PR Review: https://git.openjdk.org/jextract/pull/195#pullrequestreview-1844635774
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct).
This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted.
I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comment ------------- Changes: - all: https://git.openjdk.org/jextract/pull/195/files - new: https://git.openjdk.org/jextract/pull/195/files/726967aa..cff996eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=195&range=01 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=195&range=00-01 Stats: 25 lines in 2 files changed: 1 ins; 19 del; 5 mod Patch: https://git.openjdk.org/jextract/pull/195.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/195/head:pull/195 PR: https://git.openjdk.org/jextract/pull/195
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct).
This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted.
I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add comment ------------- Changes: - all: https://git.openjdk.org/jextract/pull/195/files - new: https://git.openjdk.org/jextract/pull/195/files/cff996eb..a85ddbed Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=195&range=02 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=195&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jextract/pull/195.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/195/head:pull/195 PR: https://git.openjdk.org/jextract/pull/195
On Thu, 25 Jan 2024 17:24:45 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This PR tweaks jextract so that typedefs of anonymous structs only generate one class (that of the struct).
This required some changes in both `NameMangler` (to make sure that the special naming rule was applied), and then in `OutputFactory`, to make sure that in such case the typedef class is not emitted.
I've tweaked a number of tests which now reflect the "less verbose" jextract output - as such I don't think we need to add further tests.
This pull request has now been integrated. Changeset: 40e19791 Author: Maurizio Cimadamore <mcimadamore@openjdk.org> URL: https://git.openjdk.org/jextract/commit/40e197911eda0e2fc1ccad0a564041b3e62a... Stats: 24 lines in 5 files changed: 13 ins; 5 del; 6 mod 7903644: typedef of anonymous struct generates a redundant class Reviewed-by: jvernee ------------- PR: https://git.openjdk.org/jextract/pull/195
participants (2)
-
Jorn Vernee
-
Maurizio Cimadamore