Skip to content

gogensig:panic when redefined #285

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

Merged
merged 18 commits into from
May 7, 2025

Conversation

luoliwoshang
Copy link
Member

@luoliwoshang luoliwoshang commented Apr 29, 2025

question 1

Tag names: all identifiers declared as names of structs, unions and enumerated types. Note that all three kinds of tags share one name space.

The following is valid because enum algorithm and typedef algorithm belong to different scopes:

typedef enum algorithm {
    UNKNOWN = 0,
    NULL = 1,
} algorithm_t;

typedef enum {
    UNKNOWN2 = 0,
    NULL2 = 1,
} algorithm_t2;

typedef algorithm_t algorithm;

If, in this case, the typedef algorithm is allowed to be mapped to another name, such as algorithm__1, it would be impossible to describe this mapping in the mapping file without causing ambiguity.

llcppg.pub

algorithm algorithm__1

The cname would correspond to two nodes: the enum node and the typedef node. Therefore, for this situation, identifying and skipping such cases would be a more appropriate choice.

typedef enum algorithm {
    UNKNOWN = 0,
    NULL = 1,
} algorithm;

In the main branch of llcppg, llcppsigfetch already supports filtering typedef self-references of this form, but now it needs to support multi-level self-references.

question 2

The following is valid but currently causes a panic:

struct isl_arg_choice {
    const char *name;
    unsigned value;
};
enum isl_arg_type {
    // ....
    isl_arg_choice
    // ....
};

The reason is that struct isl_arg_choice and the enum item isl_arg_choice should exist in different scopes, but they are currently stored in the same map[cname]pubname without generating a new name with __x based on the node type.

The isl_arg_choice as an enum item is still checked for whether its cname is in the current scope, used to submit a cname -> pubnamed type. However, for enum values, they exist as constant values and are not referenced as types in the code (e.g., syntax like isl_arg_choice x is invalid). Therefore, during the conversion process, we do not need to check the origin name of macros and enum items, but only verify their converted pubname.


  • Reduce unnecessary error interception.
  • Instead of panicking directly within the package process, return the error and let the convert function handle it.

@luoliwoshang luoliwoshang marked this pull request as draft April 29, 2025 01:51
@luoliwoshang luoliwoshang changed the title gogensig:panic when redefined [wip] gogensig:panic when redefined Apr 29, 2025
@luoliwoshang luoliwoshang marked this pull request as ready for review April 29, 2025 03:32
@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch from 11e8f73 to 5869aa1 Compare April 29, 2025 03:42
Copy link

codecov bot commented Apr 29, 2025

Codecov Report

Attention: Patch coverage is 92.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.99%. Comparing base (d01d64b) to head (cb36331).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/gogensig/convert/convert.go 72.72% 2 Missing and 1 partial ⚠️
cmd/gogensig/convert/package.go 93.61% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   80.31%   79.99%   -0.33%     
==========================================
  Files          28       22       -6     
  Lines        2957     2919      -38     
==========================================
- Hits         2375     2335      -40     
+ Misses        567      564       -3     
- Partials       15       20       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch 4 times, most recently from 89305c2 to 656ce39 Compare April 30, 2025 04:01
@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch 2 times, most recently from 73a2e5a to 8f5036e Compare April 30, 2025 06:43
@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch 3 times, most recently from 30711b9 to ee2cc1a Compare April 30, 2025 09:25
@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch from ee2cc1a to 4a6e7fb Compare April 30, 2025 09:27
Copy link

qiniu-x bot commented May 6, 2025

[Git-flow] Hi @luoliwoshang, There are some suggestions for your information:


Rebase suggestions

  • Following commits seems generated via git merge

    Merge branch 'gogensig/namemapper' into gogensig/panicredefine

    Merge remote-tracking branch 'upstream/main' into gogensig/panicredefine

Which seems insignificant, recommend to use git rebase command to reorganize your PR.

For other git-flow instructions, recommend refer to these examples.

If you have any questions about this comment, feel free to raise an issue here:

@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch from db12645 to 224ce9c Compare May 6, 2025 11:16
@luoliwoshang luoliwoshang changed the title [wip] gogensig:panic when redefined gogensig:panic when redefined May 6, 2025
@luoliwoshang luoliwoshang requested a review from MeteorsLiu May 6, 2025 11:24
@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch from 5579bb3 to ae9334a Compare May 7, 2025 04:21
@luoliwoshang luoliwoshang force-pushed the gogensig/panicredefine branch from ae9334a to cb36331 Compare May 7, 2025 04:22
@MeteorsLiu MeteorsLiu merged commit 7b8555b into goplus:main May 7, 2025
9 checks passed
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

Successfully merging this pull request may close these issues.

2 participants