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

[PATCH API-NEXT v4] Symbols cleanup #108

Closed
wants to merge 4 commits into from

Conversation

lumag
Copy link

@lumag lumag commented Aug 4, 2017

Several low-hanging fruits for https://bugs.linaro.org/show_bug.cgi?id=2988

@muvarov muvarov changed the title Symbols cleanup [PATCH API-NEXT v1] Symbols cleanup Aug 4, 2017
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Bill Fischofer bill.fischofer@linaro.org

@Bill-Fischofer-Linaro
Copy link
Contributor

Bill-Fischofer-Linaro commented Aug 4, 2017

This patch is straightforward, but I'm not sure I understand why it is necessary. We introduced the explicit visibility controls some time back to make is so that all symbols are hidden by default except those we explicitly mark as visible. Does this need to be extended here?

@muvarov
Copy link
Contributor

muvarov commented Aug 4, 2017

@Bill-Fischofer-Linaro I think in case of static library our hidden things do not work. And all names have to be in .o 's inside .a so that linked will be able to link crossing symbols.

@lumag
Copy link
Author

lumag commented Aug 4, 2017

@Bill-Fischofer-Linaro two main reasons:

  1. Static library is basically a collection of .o files, without any visibility information. Linking application statically with ODP can lead to symbol clashes.

  2. This patch set results in several cleanups thanks to cleaner inter-module interface or dead code elimination.

@muvarov
Copy link
Contributor

muvarov commented Aug 7, 2017

@lumag branch has to be against master. Why is it for api-next?

@lumag
Copy link
Author

lumag commented Aug 7, 2017 via email

@muvarov muvarov changed the title [PATCH API-NEXT v1] Symbols cleanup [PATCH API-NEXT v2] Symbols cleanup Aug 8, 2017
@lumag
Copy link
Author

lumag commented Aug 8, 2017

@muvarov @Bill-Fischofer-Linaro PR updated to reflect #116 merge

Copy link
Contributor

@brbrooks brbrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment regarding style, but otherwise looks good to me.

@@ -71,6 +71,7 @@ bool rwin_reserve(reorder_window_t *rwin, uint32_t *sn)
return true;
}

static
void rwin_insert(reorder_window_t *rwin,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coding style of this file (and majority of code base) is to place "static" on the same line as rest of function definition. Please fix this up so that it remains consistent

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brbrooks done

@muvarov muvarov changed the title [PATCH API-NEXT v2] Symbols cleanup [PATCH API-NEXT v3] Symbols cleanup Aug 8, 2017
@brbrooks
Copy link
Contributor

brbrooks commented Aug 8, 2017

Reviewed-by: Brian Brooks brian.brooks@arm.com

@@ -191,7 +191,7 @@ int odp_cls_capability(odp_cls_capability_t *capability)
return 0;
}

void _odp_cls_update_hash_proto(cos_t *cos, odp_pktin_hash_proto_t hash_proto)
static void _odp_cls_update_hash_proto(cos_t *cos, odp_pktin_hash_proto_t hash_proto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpatch flags this line as being over 80 chars in len. Needs to be split to two lines.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dmitry Eremin-Solenikov added 4 commits August 12, 2017 00:19
Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
@muvarov muvarov changed the title [PATCH API-NEXT v3] Symbols cleanup [PATCH API-NEXT v4] Symbols cleanup Aug 11, 2017
@muvarov
Copy link
Contributor

muvarov commented Aug 17, 2017

Merged.

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.

None yet

4 participants